Re: Logical Replication of sequences

Hi Vignesh,

Some review comments for patch v20250325-0002

======
Commit message

1.
Furthermore, enhancements to psql commands (\d and \dRp) now allow for better
display of publications containing specific sequences or sequences included
in a publication.

~

That doesn't seem as clear as it might be. Also, IIUC the "sequences
included in a publication" is not actually implemented yet -- there is
only the "all sequences" flag.

SUGGESTION
Furthermore, enhancements to psql commands now display which
publications contain the specified sequence (\d command), and if a
specified publication includes all sequences (\dRp command)

======
doc/src/sgml/ref/create_publication.sgml

2.
<para>
To add a table to a publication, the invoking user must have ownership
- rights on the table. The <command>FOR ALL TABLES</command> and
- <command>FOR TABLES IN SCHEMA</command> clauses require the invoking
+ rights on the table. The <command>FOR TABLES IN SCHEMA</command>,
+ <command>FOR ALL TABLES</command> and
+ <command>FOR ALL SEQUENCES</command> clauses require the invoking
user to be a superuser.

IMO these should all be using <literal> SGML markup same as elsewhere
on this page, not <command> markup.

======
src/backend/commands/publicationcmds.c

3.
if (!superuser_arg(newOwnerId))
{
if (form->puballtables)
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be
a superuser."));
if (form->puballsequences)
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL SEQUENCES publication must
be a superuser."));
if (is_schema_publication(form->oid))
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR TABLES IN SCHEMA publication
must be a superuser."));
}

I wondered if there's too much duplicated code here. Maybe it's better
to share a common ereport?

SUGGESTION

if (!superuser_arg(newOwnerId))
{
char *hint_msg = NULL;

if (form->puballtables)
hint_msg = _("The owner of a FOR ALL TABLES publication must be a
superuser.");
else if (form->puballsequences)
hint_msg = _("The owner of a FOR ALL SEQUENCES publication must be
a superuser.");
else if (is_schema_publication(form->oid))
hint_msg = _("The owner of a FOR TABLES IN SCHEMA publication must
be a superuser.");
if (hint_msg)
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint(hint_msg));
}

======
src/bin/psql/describe.c

describeOneTableDetails:

4.
+ res = PSQLexec(buf.data);
+ if (!res)
+ goto error_return;
+
+ numrows = PQntuples(res);
+

Isn't this same code already done a few lines above in the same
function? Maybe I misread something.

======
src/test/regress/sql/publication.sql

5.
+-- check that describe sequence lists all publications the sequence belongs to

Might be clearer to say: "lists both" instead of "lists all"

======
Kind Regards,
Peter Smith.
Fujitsu Australia