Re: Logical Replication of sequences - Mailing list pgsql-hackers

Hi Vignesh.

Here are some review comments for patch v20241230-0002

======
1. SYNTAX

The proposed syntax is currently:

CREATE PUBLICATION name
    [ FOR ALL object_type [, ...]
      | FOR publication_object [, ... ] ]
    [ WITH ( publication_parameter [= value] [, ... ] ) ]

where object_type is one of:

    TABLES
    SEQUENCES

where publication_object is one of:

    TABLE [ ONLY ] table_name [ * ] [ ( column_name [, ... ] ) ] [
WHERE ( expression ) ] [, ... ]
    TABLES IN SCHEMA { schema_name | CURRENT_SCHEMA } [, ... ]
~

But lately, I've been thinking it could be clearer if you removed the
object_type and instead fully spelled out FOR ALL TABLES and/or FOR
ALL SEQUENCES.

compare
CREATE PUBLICATION FOR ALL TABLES, SEQUENCES;
versus
CREATE PUBLICATION FOR ALL TABLES, ALL SEQUENCES;

~

Also AFAICT, the current syntax says it is impossible to mix FOR ALL
SEQUENCES with FOR TABLE etc but really that *should* be allowed,
right?

And it looks like you may come to similar grief in future if you try
things like:
"FOR ALL TABLES" mixed with "FOR SEQUENCE seq_name"
"FOR ALL TABLES" mixed with "FOR SEQUENCES IN SCHEMA schema_name"

~

So, maybe a revised syntax like below would end up being easier and
also more flexible:

CREATE PUBLICATION name
    [ FOR publication_object [, ... ] ]
    [ WITH ( publication_parameter [= value] [, ... ] ) ]

where publication_object is one of:

    ALL TABLES
    ALL SEQUENCES
    TABLE [ ONLY ] table_name [ * ] [ ( column_name [, ... ] ) ] [
WHERE ( expression ) ] [, ... ]
    TABLES IN SCHEMA { schema_name | CURRENT_SCHEMA } [, ... ]

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

CreatePublication:

2.
- /* FOR ALL TABLES requires superuser */
- if (stmt->for_all_tables && !superuser())
+ /* FOR ALL TABLES or FOR ALL SEQUENCES requires superuser */
+ if ((stmt->for_all_tables || stmt->for_all_sequences) && !superuser())
  ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create FOR ALL TABLES publication")));
+ errmsg("must be superuser to create ALL TABLES and/or ALL SEQUENCES
publication")));


2a.
Typo.

/create ALL TABLES and/or ALL SEQUENCES publication/create a FOR ALL
TABLES and/or a FOR ALL SEQUENCES publication/

~

2b.
This message might be OK now, but I suspect it will become very messy
in future after you introduce another syntax like "FOR SEQUENCE
seq_name" etc (which would also be able to be used in combination with
a FOR ALL TABLES).

So, I think that for future-proofing against all the possible (future)
combinations, and for keeping the code cleaner, it will be far simpler
to just keep the errors for tables and sequences separated:

SUGGESTION:
if (!superuser())
{
  if (stmt->for_all_tables)
    ereport(ERROR, ... FOR ALL TABLES ...);
  if (stmt->for_all_sequences)
    ereport(ERROR, ... FOR ALL SEQUENCES ...);
}

~~~

AlterPublicationOwner_internal:

3.
- if (form->puballtables && !superuser_arg(newOwnerId))
+ /* FOR ALL TABLES or FOR ALL SEQUENCES requires superuser */
+ if ((form->puballtables || form->puballsequences) &&
+ !superuser_arg(newOwnerId))
  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.")));
+ errhint("The owner of ALL TABLES and/or ALL SEQUENCES publication
must be a superuser.")));

Ditto the above comment #2.

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

4.
+ puboid_col = cols++;
+ pubname_col = cols++;
+ pubowner_col = cols++;
+ puballtables_col = cols++;
+
+ if (has_pubsequence)
+ {
+ appendPQExpBufferStr(&buf,
+ ", puballsequences");
+ puballsequences_col = cols++;
+ }
+
+ appendPQExpBufferStr(&buf,
+ ", pubinsert, pubupdate, pubdelete");
+ pubins_col = cols++;
+ pubupd_col = cols++;
+ pubdel_col = cols++;
+
  if (has_pubtruncate)
+ {
  appendPQExpBufferStr(&buf,
  ", pubtruncate");
+ pubtrunc_col = cols++;
+ }
+
  if (has_pubgencols)
+ {
  appendPQExpBufferStr(&buf,
  ", pubgencols");
+ pubgen_col = cols++;
+ }
+
  if (has_pubviaroot)
+ {
  appendPQExpBufferStr(&buf,
  ", pubviaroot");
+ pubviaroot_col = cols++;
+ }

There is some overlap/duplication of the new variable 'cols' and the
existing variable 'ncols'.

AFAICT you can just move/replace the declaration of 'ncols' to where
'cols' is declared, and then you can remove the duplicated code below
(because the above code is already doing the same thing).

if (has_pubtruncate)
  ncols++;
if (has_pubgencols)
  ncols++;
if (has_pubviaroot)
  ncols++;
if (has_pubsequence)
  ncols++;

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