| From: | Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
|---|---|
| To: | Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> |
| Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Support EXCEPT for ALL SEQUENCES publications |
| Date: | 2026-07-01 08:30:06 |
| Message-ID: | CANhcyEWtAUnHF-3MKTSmH+j-A9sQSfuTdcCA9K285bL1ALmnyw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Tue, 30 Jun 2026 at 13:17, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Jun 30, 2026 at 12:14 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Mon, 29 Jun 2026 at 06:02, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shlok.
> > >
> > > I only had one general comment about v14. IMO, it would be better to
> > > try to make the regression test object names more meaningful where
> > > possible (though sometimes it won't be). The xxx1, xxx2, and xxx3
> > > become harder to read as more gets added.
> > >
> > > e.g. `regress_pub_seq3` -- What does 'pub' mean here? This is an
> > > unlogged sequence you want to exclude.
> > > e.g. `regress_pub_forallsequences4` -- Doesn't mean much.
> > > e.g. `tab_seq` -- This is meant to be a plain table; Not a table
> > > pretending to be a seq.
> > >
> > > Below is an example of some modifications, but there are many more.
> > > Consider checking all the names to see if they can be improved
> > > (sometimes you may be stuck having to accommodate existing names).
> > > (same comment applies to both patches).
> > >
> > > ~~~
> > >
> > > BEFORE
> > > +-- fail - unlogged sequence is specified in EXCEPT sequence list
> > > +CREATE UNLOGGED SEQUENCE regress_pub_seq3;
> > > +CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
> > > EXCEPT (SEQUENCE regress_pub_seq3);
> > > +
> > > +-- fail - temporary sequence is specified in EXCEPT sequence list
> > > +CREATE TEMPORARY SEQUENCE regress_pub_seq4;
> > > +CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
> > > EXCEPT (SEQUENCE regress_pub_seq4);
> > > +
> > > +-- fail - sequence object is specified in EXCEPT table list
> > > +CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL TABLES EXCEPT
> > > (TABLE regress_pub_seq0);
> > > +
> > > +-- fail - table object is specified in EXCEPT sequence list
> > > +CREATE TABLE tab_seq(a int);
> > > +CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
> > > EXCEPT (SEQUENCE tab_seq);
> > > +
> > >
> > >
> > > SUGGESTION
> > > +-- fail - unlogged sequence is specified in EXCEPT sequence list
> > > +CREATE UNLOGGED SEQUENCE regress_seq_unlogged;
> > > +CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
> > > (SEQUENCE regress_seq_unlogged);
> > > +
> > > +-- fail - temporary sequence is specified in EXCEPT sequence list
> > > +CREATE TEMPORARY SEQUENCE regress_seq_temp;
> > > +CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
> > > (SEQUENCE regress_seq_temp);
> > > +
> > > +-- fail - sequence object is specified in EXCEPT table list
> > > +CREATE PUBLICATION regress_pub_should_fail FOR ALL TABLES EXCEPT
> > > (TABLE regress_seq1);
> > > +
> > > +-- fail - table object is specified in EXCEPT sequence list
> > > +CREATE TABLE tab1(a int);
> > > +CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
> > > (SEQUENCE tab1);
> > > +
> > >
> > I agree with your suggestions, I have made the changes.
> > I have also made changes for other objects as well in publication.sql
> > and 037_except.pl files.
> >
> > I also agree with the suggestions by Shveta in [1] and made the
> > required changes.
> >
> > Please find the updated v15 patch attached.
> >
>
> + /* TODO : Add a version check for PG 20 */
> + if (pset.sversion >= 190000)
> + {
> + /* Get sequences in the EXCEPT clause for this publication */
> + printfPQExpBuffer(&buf, "/* %s */\n",
> + _("Get sequences excluded by this publication"));
> + printfPQExpBuffer(&buf,
> + "SELECT n.nspname || '.' || c.relname\n"
> + "FROM pg_catalog.pg_class c\n"
> + " JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n"
> + " JOIN pg_catalog.pg_publication_rel pr ON c.oid = pr.prrelid\n"
> + "WHERE pr.prpubid = '%s' AND pr.prexcept AND c.relkind = 'S'\n"
> + "ORDER BY 1", pubid);
> + if (!addFooterToPublicationDesc(&buf, _("Except sequences:"),
> + true, &cont))
> + goto error_return;
> + }
> + }
>
> I've few questions related to above change:
>
> 1) Should the second printfPQExpBuffer() be appendPQExpBuffer()? As
> written, it overwrites the query comment added just above, so psql -E
> will not show "Get sequences excluded by this publication".
>
I agree, the second printfPQExpBuffer() should be appendPQExpBuffer().
Made the change.
> 2) The above query may work on PG 19, but since the feature is going
> to be part of PG20, why do you want this query to be part of PG19?
>
I added the checks for PG19, as the major version on master was still
pointing to PG19.
But now I noticed that the major version is now bumped to PG20, I have
modified the checks
> --
>
> Do we also need to include a tap test for "ALTER PUBLICATION ... SET
> ALL SEQUENCES EXCEPT (...)" followed by "ALTER SUBSCRIPTION ...
> REFRESH SEQUENCES", and then sequence replication behavior is
> verified?
>
Yes, I agree
I have made the changes and attached the updated v16 patches. I have
also addressed Peter's comment in [1].
Thanks,
Shlok Kyal
| Attachment | Content-Type | Size |
|---|---|---|
| v16-0001-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLI.patch | application/octet-stream | 66.5 KB |
| v16-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLIC.patch | application/octet-stream | 33.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Bertrand Drouvot | 2026-07-01 08:38:16 | Re: Add pg_stat_kind_info system view |
| Previous Message | Peter Eisentraut | 2026-07-01 08:21:50 | Re: pg_createsubscriber --dry-run logging concerns |