Re: Logical Replication of sequences

>
> Some review comments for patch 0001:
> 1. In src/backend/commands/sequence.c
> in pg_sequence_state()
> + /* open and lock sequence */
> + init_sequence(seq_relid, &elm, &seqrel);
> +
> + if (pg_class_aclcheck(elm->relid, GetUserId(),
> +  ACL_SELECT | ACL_USAGE) != ACLCHECK_OK)
> + ereport(ERROR,
> + errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("permission denied for sequence %s",
> +   RelationGetRelationName(seqrel)));
> +
>
> How about using aclcheck_error for this, which also supports error messages for specific access errors. Most other
objectsseem to be using this.
 
> if (aclresult != ACLCHECK_OK)
> aclcheck_error(aclresult, get_relkind_objtype(seqrel->relkind),
>   RelationGetRelationName(seqrel));

I felt this is ok in this case as it is used similarly in
nextval_internal, currval_oid, lastval, do_setval and
pg_sequence_parameters also

> 2. in function pg_sequence_state()
> +
> + UnlockReleaseBuffer(buf);
> + relation_close(seqrel, NoLock);
>
> Ideally the corresponding close for init_sequence is sequence_close() rather than relation_close()

Fixed

The comment for the same is handled in the v2025029 version patch
attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0ssEaHW8by5kd1%3DwE7LPMhhBiV6971JbFWsY6Qwp7NMw%40mail.gmail.com

Regards,
Vignesh