On Friday, February 10, 2023 7:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
>
> Comments on 0001 and 0002
> =======================
> 1.
> * CREATE COLLATION
> */
> ObjectAddress
> -DefineCollation(ParseState *pstate, List *names, List *parameters,
> bool if_not_exists)
> +DefineCollation(ParseState *pstate, List *names, List *parameters,
> + bool if_not_exists, ObjectAddress *from_existing_collid)
>
> I think it is better to expand function header comments to explain
> return values especially from_existing_collid.
Added.
> 2.
> +Form_pg_sequence_data
> +get_sequence_values(Oid sequenceId)
> +{
> + Buffer buf;
> + SeqTable elm;
> + Relation seqrel;
> + HeapTupleData seqtuple;
> + Form_pg_sequence_data seq;
> + Form_pg_sequence_data retSeq =
> palloc(sizeof(FormData_pg_sequence_data));
> +
> + /* Open and AccessShareLock sequence */
> + init_sequence(sequenceId, &elm, &seqrel);
>
> The comment above init_sequence() seems wrong to me. AFAICS, we
> acquire RowExclusiveLock in init_sequence() via
> lock_and_open_sequence(). Am, I missing anything?
Changed.
> 3.
> +get_sequence_values(Oid sequenceId)
> ...
> ...
> +
> + if (pg_class_aclcheck(sequenceId, GetUserId(),
> + ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>
> Why do we need to check UPDATE privilege just for reading the values?
I think it was mis-copied, removed.
>
> 4. I checked the callers of get_sequence_values and they just need
> 'last_val' but we still expose all values from Form_pg_sequence_data,
> not sure if that is required. In deparse_CreateSeqStmt(), we use it to
> append RESTART but the CREATE SEQUENCE syntax in docs doesn't have a
> RESTART clause, so I am confused as to why the patch appends the same.
> If it is really required then please add the comments for the same.
From the code, it seems CREATE SEQUENCE supports the RESTART keyword while the
document doesn’t mention it. I think I will start a separate thread to discuss
this.
>
> 5. In deparse_CreateSeqStmt() and deparse_ColumnIdentity(), we open
> SequenceRelationId, is that really required? Isn't locking the
> sequence relation sufficient as is done by get_sequence_values()?
> Also, I see that deparse_CreateSeqStmt() opens and locks the sequence
> whereas deparse_ColumnIdentity() doesn't do the same. Then, we unlock
> the relation in some cases but not in others (like get_sequence_values
> uses NoLock whereas others release the lock while closing the rel).
>
> 6. In get_sequence_values(), we check the permissions whereas callers
> just assume that it is done and don't check it while accessing the
> sequence. This makes the code a bit unclear.
>
> 7. After seeing the above inconsistencies, I am thinking will it be
> better to design get_sequence_values() such that it returns both
> sequence parameters and last_val in a structure and the callers use
> it. That would bit clean and avoid opening the relation multiple
> times.
Agreed. Refactored this as suggested.
>
> 8.
> +/*
> + * Return the given object type as a string.
> + * If isgrant is true, then this function is called
> + * while deparsing GRANT statement and some object
> + * names are replaced.
> + */
> +const char *
> +stringify_objtype(ObjectType objtype, bool isgrant)
>
> Have an empty line after the Return line. The line length appears too short.
Adjusted.
>
> 9. Similar to stringify_grant_objtype() and
> stringify_adefprivs_objtype(), shall we keep the list of all
> unsupported types in stringify_objtype()? That will help us to easily
> identify which objects are yet not supported.
Added.
>
> 10. In pg_get_ruledef_detailed(), the code to form a string for qual
> and action is mostly the same as what we have in make_ruledef(). I
> think we can extract the common code into a separate function to avoid
> missing the code updates at one of the places. I see that
> 'special_exprkind' is present in one place and not in other, it may be
> that over time, we have added new things to deparse_context which
> doesn't get updated in the patch. Also, I noticed that for
> CMD_NOTHING, the patch just ignores the action whereas the core code
> does append the definition. We should check whether such a difference
> is required and if so, then add comments for the same.
I extracted the command code into a separate function as suggested,
and fixed these inconsistences.
Here is the new version patch which addressed above comments.
I also fixed a bug for the deparsing of CREATE RULE that it didn't add
parentheses for rule action list.
And thanks for Vignesh to help addressing the comments.
Best Regards,
Hou zj