MIME-Version: 1.0
References: 
 <CAHut+Pti83yGaV5-DZU=AvJHxFDuoKW8_pjSedRham8SgZxLYA@mail.gmail.com>
 <CALDaNm0nLdBKJVHVvvOnY_5mkVg20=OL18fdjA5+KZ3GhPB=TQ@mail.gmail.com>
 <CAHut+Pv+a-7-NRrZv4v6RfaaUo5b21RXe0tGOu8CfKrxPjE=tw@mail.gmail.com>
 <CAJpy0uAHe88hL5MX3q9tyGyx_gCKKHcWnmETXvXM2CqnE8jrmA@mail.gmail.com>
 <CAHut+PtfHzxHFkHJWYoxOFpgpSH5HNAGmP5sSrwh1d+R0Ab-BQ@mail.gmail.com>
 <CAHut+PsjrPOcs=ePWm+N-q=rdmhDeM-FE05gPgDoN647Jb3RaQ@mail.gmail.com>
In-Reply-To: 
 <CAHut+PsjrPOcs=ePWm+N-q=rdmhDeM-FE05gPgDoN647Jb3RaQ@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 9 Apr 2026 09:39:35 +0530
Message-ID: 
 <CAJpy0uA9rj1xDDot3ydn_DZERH4Mc8D70syE3Xg8LWfS8zanZg@mail.gmail.com>
Subject: Re: Logical Replication - revisit `is_table_publication` function
 implementation
To: Peter Smith <smithpb2250@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uA9rj1xDDot3ydn_DZERH4Mc8D70syE3Xg8LWfS8zanZg%40mail.gmail.com>
Precedence: bulk

On Wed, Apr 8, 2026 at 11:58=E2=80=AFAM Peter Smith <smithpb2250@gmail.com>=
 wrote:
>
> On Wed, Apr 8, 2026 at 4:04=E2=80=AFPM Peter Smith <smithpb2250@gmail.com=
> wrote:
> >
> > On Wed, Apr 8, 2026 at 3:25=E2=80=AFPM shveta malik <shveta.malik@gmail=
.com> wrote:
> > >
> > > On Wed, Apr 8, 2026 at 10:24=E2=80=AFAM Peter Smith <smithpb2250@gmai=
l.com> wrote:
> > > >
> > > > On Wed, Apr 8, 2026 at 1:45=E2=80=AFPM vignesh C <vignesh21@gmail.c=
om> wrote:
> > > > >
> ...
> > > > > And also why just check for puballtables why not to check for pub=
allsequences
> > > >
> > > > I think function is_schema_publication() is unrelated to 'puballseq=
uences'.
> > > >
> > > > e.g. all the following will still need to check
> > > > pg_publication_namespace, regardless of the 'puballsequences' value=
.
> > > >
> > > > ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
> > > > ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA=
 s1;
> > > > ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;
> > > >
> > >
> > > IIUC, we don't support mix of ALL SEQUENCES and TABLES IN SCHEMA s1.
> > > So I could not understand your point, why FOR ALL SEQ still need to
> > > check pg_publication_namespace?
> > >
> >
> > Oh! You are right.
> >
> > (Sorry, Vignesh, I did not recognise that combination as unsupported).
> >
> > I'll post a patch update to handle it.
> >
>
> PSA patch v2.
>
> Same as before, but now also doing a quick return false from both
> functions if `puballsequences` is true.
>

Okay. I was trying to determine where this optimization would be beneficial=
.

In cases, where we attempt to add tables or schemas to an ALL TABLES
or ALL SEQUENCES publication, the operation will error out in
CheckAlterPublication() before is_table_publication() or
is_schema_publication() are even called. And in cases where we are
trying to add table or schema to a non ALL-TABLEs/SEQ pub, and we end
up invoking these functions, we still need to traverse pg_pub_rel.

The only scenario (as I understand it) that benefits from this change
is when we try to add EXCEPT to an ALL TABLES publication. In that
case, both of the concerned functions would not need to access
pg_pub_rel if the publication is already an ALL TABLES publication. So
this optimization helps in a positive (non-erroneous) case.

In cases where we need to throw an error (for example, adding EXCEPT
to a FOR TABLE publication), these checks would not provide any
benefit as we still need to traverse pg_pub_rel to see if it has any
valid tables or it is an emty publication (empty one is fine).

But since the optimization improves a valid, non-erroneous scenario,
IMO, it is good to include it.  Let's see what others have to say on
this.

thanks
Shveta