Re: Support EXCEPT for ALL SEQUENCES publications

On Wed, 24 Jun 2026 at 15:21, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Tue, Jun 23, 2026 at 3:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> 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