On Thu, Jun 25, 2026 at 2:17 PM Shlok Kyal <[email protected]> wrote:
>
> 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.
>
Okay, thanks for clarifying. I am surprised that SEQUENCE is not a
reserved keyword.
postgres=# create sequence sequence;
CREATE SEQUENCE
Anyway, currnet implementation is fine then.
> >
> > 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.
>
Okay, thanks for the details.
thanks
Shveta
