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