Re: Logical Replication of sequences

On Fri, 3 Jan 2025 at 06:53, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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 } [, ... ]

The proposed be more easier to extend the syntax in the future, so
modified.The proposed be more easier to extend the syntax in the
future, so modified.

> ======
> 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/

Modified

> ~
>
> 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 ...);
> }

If we do that way it will not print both the stmt publication type if
both "ALL TABLES" and "ALL SEQUENCES" is specified.

> ~~~
>
> 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.

Modified the message

> ======
> 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++;

I have removed ncols and used cols.

The attached patch has the changes for the same.

Regards,
Vignesh