On Wed, 24 Jun 2026 at 15:21, shveta malik <[email protected]> wrote: > > On Tue, Jun 23, 2026 at 3:03 PM Shlok Kyal <[email protected]> wrote: > > > > > > I have addressed the comments and attached the updated v12 patch > > > > Thanks Shlok. I have resumed review of this thread. A few comment for 001: > > > 1) > > Can we make 'opt_sequence' (SEQUENCE | EMPTY) similar to opt_table and > then use it here. It will be similar to pub_except_tbl_list and easier > to understand. > The compiler cannot compile if I use a syntax similar to 'pub_except_tbl_list'. I tried making it like: pub_except_seq_list: PublicationExceptSeqSpec
{ $$ = list_make1($1); }
| pub_except_seq_list ','
opt_sequence PublicationExceptSeqSpec
{ $$ = lappend($1, $4); } ;
Getting error as:
gram.y: error: shift/reduce conflicts: 1 found, 0 expected
gram.y: warning: shift/reduce conflict on token SEQUENCE
[-Wcounterexamples] First example: pub_except_seq_list • SEQUENCE
PublicationExceptSeqSpec Shift derivation pub_except_seq_list ↳ 1513:
pub_except_seq_list opt_sequence PublicationExceptSeqSpec ↳ 1855: •
SEQUENCE Second example: pub_except_seq_list • SEQUENCE Reduce
derivation pub_except_seq_list ↳ 1513: pub_except_seq_list
opt_sequence SEQUENCE ↳ 1856: ε •
This syntax is not able to compile because it is getting confused:
For example: ".... EXCEPT (SEQUENCE s1, SEQUENCE ... )"
For the second 'SEQUENCE', it's unclear whether we should treat
'SEQUENCE' as a sequence_name or a keyword.
To avoid the error, I wrote it the way in the patch.
'opt_table' does not face the same issue because 'TABLE' is a reserved
keyword, but 'SEQUENCE' is not.
> 2)
> getPublications:
> + if (relkind == RELKIND_SEQUENCE)
> + simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
> + else
> + simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
>
> Can we do this:
> if (relkind == RELKIND_SEQUENCE)
> ...
> else if (relkind == RELKIND_RELATION ||
> relkind == RELKIND_PARTITIONED_TABLE)
> ...
> else
> Assert(false);
>
>
> 3)
> describeOneTableDetails:
> + * Skip entries where this sequence appears in the publication's
> + * EXCEPT list.
> + *
> + * For a FOR ALL SEQUENCES publication, pg_publication_rel
> + * contains entries only for sequences specified in the EXCEPT
> + * clause, so there is no need to check pr.prexcept explicitly.
>
> My personal preference will be to have pr.prexcept check in the query
> and get rid of this comment.
>
> 4)
> - char *footers[3] = {NULL, NULL, NULL};
> + char *footers[4] = {NULL, NULL, NULL, NULL};
>
> Is this intentional? We are using only 3 of these though.
Yes, this is intentional. The last element of the footers array must
always be NULL because printQuery() treats it as a NULL-terminated
array:
/* set footers */
if (opt->footers)
{
char **footer;
for (footer = opt->footers; *footer; footer++)
printTableAddFooter(&cont, *footer);
}
The loop terminates only when it encounters a NULL entry, so the last
array element must remain NULL.
This is also consistent with the existing code. For example, in describe.c:
if (tableinfo.relkind == RELKIND_PROPGRAPH)
{
printQueryOpt popt = pset.popt;
char *footers[3] = {NULL, NULL, NULL};
only footers[0] and footers[1] are populated, while footers[2] serve
as the required terminating NULL.
I have addressed the remaining comments and attached the updated v13 patches
I have also addressed Peter's comments in [1].
[1]:
https://www.postgresql.org/message-id/CAHut%2BPvMXOR73%2BWdvSWd3B8j6jDQQXRtO_bKEudi76n30GAApQ%40mail.gmail.com
Thanks,
Shlok Kyal
v13-0001-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLI.patch
Description: Binary data
v13-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLIC.patch
Description: Binary data
