Subject: Re: bogus: logical replication rows/cols combinations

From: Alvaro Herrera <[email-masked]>
Date: Sat, 30 Apr 2022 11:31:04 +0200

On 2022-Apr-30, Amit Kapila wrote:

> On Sat, Apr 30, 2022 at 2:02 AM Tomas Vondra
> <[email-masked]> wrote:

> > That seems to deal with a circular replication, i.e. two logical
> > replication links - a bit like a multi-master. Not sure how is that
> > related to the issue we're discussing here?
> 
> It is not directly related to what we are discussing here but I was
> trying to emphasize the point that users need to define the logical
> replication via pub/sub sanely otherwise they might see some weird
> behaviors like that.

I agree with that.

My proposal is that if users want to define multiple publications, and
their definitions conflict in a way that would behave ridiculously (==
bound to cause data inconsistencies eventually), an error should be
thrown.  Maybe we will not be able to catch all bogus cases, but we can
be prepared for the most obvious ones, and patch later when we find
others.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

---

From: Amit Kapila <[email-masked]>
Date: Sat, 30 Apr 2022 15:41:52 +0530

> be prepared for the most obvious ones, and patch later when we find

I agree with throwing errors for obvious/known bogus cases but do we
want to throw errors or restrict the combining of column lists when
row filters are present in all cases? See some examples [1 ] where it
may be valid to combine them.

[1] - https://www.postgresql.org/message-id/CAA4eK1K%2BPkkC6_FDemGMC_i%2BAakx%2B3%3DQG-g4We3BdCK7dK_bgA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 30 Apr 2022 18:40:46 +0200

> may be valid to combine them.

I agree we should try to combine things when it is sensible to do so.
Another case that may make sense if there are two or more publications
with identical column lists but different row filters -- in such cases,
as Tomas suggested, we should combine the filters with OR.

Also, if only INSERTs are published and not UPDATE/DELETEs, then it
might be sensible to combine everything, regardless of whether or not
the column lists and row filters match.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 1 May 2022 23:57:16 +0200

> may be valid to combine them.

I think there are three challenges:

(a) Deciding what's an obvious bug or an unsupported case (e.g. because
it's not clear what's the correct behavior / way to merge column lists).

(b) When / where to detect the issue.

(c) Making sure this does not break/prevent existing use cases.

As I said before [1], I think the issue stems from essentially allowing
DML to have different row filters / column lists. So we could forbid
publications to specify WITH (publish=...) and one of the two features,
or make sure subscription does not combine multiple such publications.

The second option has the annoying consequence that it makes this
useless for the "data redaction" use case I described in [2], because
that relies on combining multiple publications.

Furthermore, what if the publications change after the subscriptions get
created? Will we be able to detect the error etc.?

So I'd prefer the first option, but maybe that prevents some useful use
cases too ...

regards

[1]
https://www.postgresql.org/message-id/45d27a8a-7c7a-88e8-a3db-c7c1d144df5e%40enterprisedb.com

[2]
https://www.postgresql.org/message-id/338e719c-4bc8-f40a-f701-e29543a264e4%40enterprisedb.com

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 May 2022 11:01:13 +0530

> publications to specify WITH (publish=...) and one of the two features,

I don't think this is feasible for row filters because that would mean
publishing all actions because we have a restriction that all columns
referenced in the row filter expression are part of the REPLICA
IDENTITY index. This restriction is only valid for updates/deletes, so
if we allow all pubactions then this will be imposed on inserts as
well. A similar restriction is there for column lists as well, so I
don't think we can do it there as well. Do you have some idea to
address it?

> or make sure subscription does not combine multiple such publications.

Yeah, or don't allow to define such publications in the first place so
that different subscriptions can't combine them but I guess that might
forbid some useful cases as well where publication may not get
combined with other publications.

> that relies on combining multiple publications.

True, but as a workaround users can create different subscriptions for
different publications.

> created? Will we be able to detect the error etc.?

I think from that apart from 'Create Subscription', the same check
needs to be added for Alter Subscription ... Refresh, Alter
Subscription ... Enable.

In the publication side, we need an additional check in Alter
Publication ... SET table variant. One idea is that we get all other
publications for which the corresponding relation is defined. And then
if we find anything which we don't want to allow then we can throw an
error. This will forbid some useful cases as well as mentioned above.
So, the other possibility is to expose all publications for a
walsender, and then we can find the exact set of publications where
the current publication is used with other publications and we can
check only those publications. So, if we have three walsenders
(walsnd1: pub1, pub2; walsnd2 pub2; walsnd3: pub2, pub3) in the system
and we are currently altering publication pub1 then we need to check
only pub3 for any conflicting conditions. Yet another simple way could
be that we don't allow to change column list via Alter Publication ...
Set variant because the other variants anyway need REFRESH publication
which we have covered.

I think it is tricky to decide what exactly we want to forbid, so, we
may want to follow something simple like if the column list and row
filters for a table are different in the required set of publications
then we treat it as an unsupported case. I think this will prohibit
some useful cases but should probably forbid the cases we are worried
about here.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 May 2022 11:06:23 +0530

> publishing all actions because we have a restriction that all columns

Read the above sentence as: "publishing all actions and we have a
restriction that all columns ..."

> only pub3 for any conflicting conditions.
>

Typo, it should be pub2 instead of pub3 in the above sentence.

> Yet another simple way could

-- 
With Regards,
Amit Kapila.

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 2 May 2022 11:35:00 +0200

> don't think we can do it there as well. Do you have some idea to

No, I haven't thought about how exactly to implement this, and I have
not thought about how to deal with the replica identity issues. My
thoughts were that we'd only really need this for tables with row
filters and/or column lists, treating it as a cost of those features.

But yeah, it seems annoying.

> combined with other publications.

But how would you check that? You don't know which publications will be
combined by a subscription until you create the subscription, right?

> True, but as a workaround users can create different subscriptions for

Won't that replicate duplicate data, when the row filters re not
mutually exclusive?

> some useful cases but should probably forbid the cases we are worried

I don't have a clear idea on what the right tradeoff is :-(

Maybe we're digressing a bit from the stuff Alvaro complained about
initially. Arguably the existing column list behavior is surprising and
would not work with reasonable use cases. So let's fix it.

But maybe you're right validating row filters is a step too far. Yes,
users may define strange combinations of publications, but is that
really an issue we have to solve?

regards

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 May 2022 12:17:54 +0200

> combined by a subscription until you create the subscription, right?

... and I think this poses a problem: if the publisher has multiple
publications and the subscriber later uses those to create a combined
subscription, we can check at CREATE SUBSCRIPTION time that they can be
combined correctly.  But if the publisher decides to change the
publications changing the rules and they are no longer consistent, can
we throw an error at ALTER PUBLICATION point?  If the publisher can
detect that they are being used together by some subscription, then
maybe we can check consistency in the publication side and everything is
all right.  But I'm not sure that the publisher knows who is subscribed
to what, so this might not be an option.

The latter ultimately means that we aren't sure that a combined
subscription is safe.  And in turn this means that a pg_dump of such a
database cannot be restored (because the CREATE SUBSCRIPTION will be
rejected as being inconsistent).

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 2 May 2022 12:23:16 +0200

> to what, so this might not be an option.

AFAIK we don't track that (publication/subscription mapping). The
publications are listed in publication_names parameter of the
START_REPLICATION command.

> rejected as being inconsistent).

We could do this check when executing the START_REPLICATION command, no?

regards

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 May 2022 12:55:10 +0200

> We could do this check when executing the START_REPLICATION command, no?

Ah!  That sounds like it might work: we throw WARNINGs are CREATE
SUBSCRIPTION (so that users are immediately aware in case something is
going to fail later, but the objects are still created and they can fix
the publications afterwards), but the real ERROR is in START_REPLICATION.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 May 2022 16:44:58 +0530

> >> combined by a subscription until you create the subscription, right?

Yeah, I was thinking to check for all publications where the same
relation is published but as mentioned that may not be a very good
option as that would unnecessarily block many valid cases.

> START_REPLICATION command.

We don't do that currently but we can as mentioned in my previous
email [1]. Let me write the relevant part again. We need to expose all
publications for a walsender, and then we can find the exact set of
publications where the current publication is used with other
publications and we can check only those publications. So, if we have
three walsenders (walsnd1: pub1, pub2; walsnd2 pub2; walsnd3: pub2,
pub3) in the system and we are currently altering publication pub1
then we need to check only pub3 for any conflicting conditions.

I think it is possible to expose a list of publications for each
walsender as it is stored in each walsenders
LogicalDecodingContext->output_plugin_private. AFAIK, each walsender
can have one such LogicalDecodingContext and we can probably share it
via shared memory?

[1] - https://www.postgresql.org/message-id/CAA4eK1LGX-ig%3D%3DQyL%2B%3D%3DnKvcAS3qFU7%3DNiKL77ukUT-Q_4XncQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 2 May 2022 16:53:33 +0530

> Won't that replicate duplicate data, when the row filters re not

True, but this is a recommendation for mutually exclusive data, and as
far as I can understand the example given by you [1] and Alvaro has
mutually exclusive conditions. In your example, one of the
publications has a condition (region = 'USA') and the other
publication has a condition (region != 'USA'), so will there be a
problem in using different subscriptions for such cases?

[1] - https://www.postgresql.org/message-id/[email-masked]

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 May 2022 13:44:15 +0200

> then we need to check only pub3 for any conflicting conditions.

Hmm ... so what happens in the current system, if you have a running
walsender and modify the publication concurrently?  Will the subscriber
start getting the changes with the new publication definition, at some
arbitrary point in the middle of their stream?  If that's what we do,
maybe we should have a signalling system which disconnects all
walsenders using that publication, so that they can connect and receive
the new definition.

I don't see anything in the publication DDL that interacts with
walsenders -- perhaps I'm overlooking something.

> can have one such LogicalDecodingContext and we can probably share it

I guess we need to create a DSM each time a walsender opens a
connection, at START_REPLICATION time.  Then ALTER PUBLICATION needs to
connect to all DSMs of all running walsenders and see if they are
reading from it.  Is that what you have in mind?  Alternatively, we
could have one DSM per publication with a PID array of all walsenders
that are sending it (each walsender needs to add its PID as it starts).
The latter might be better.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 2 May 2022 19:36:54 +0200

> walsenders -- perhaps I'm overlooking something.

pgoutput.c is relies on relcache callbacks to get notified of changes.
See the stuff that touches replicate_valid and publications_valid. So
the walsender should notice the changes immediately.

Maybe you have some particular case in mind, though?

> The latter might be better.

I don't quite follow what we're trying to build here. The walsender
already knows which publications it works with - how else would
pgoutput.c know that? So the walsender should be able to validate the
stuff it's supposed to replicate is OK.

Why would we need to know publications replicated by other walsenders?
And what if the subscriber is not connected at the moment? In that case
there'll be no walsender.

regards

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 2 May 2022 19:51:54 +0200

> the walsender should notice the changes immediately.

Hmm, I suppose that makes any changes easy enough to detect.  We don't
need a separate signalling mechanism.

But it does mean that the walsender needs to test the consistency of
[rowfilter, column list, published actions] whenever they change for any
of the current publications and it is working for more than one, and
disconnect if the combination no longer complies with the rules.  By the
next time the replica tries to connect, START_REPLICATION will throw the
error.

> there'll be no walsender.

Sure, if the replica is not connected then there's no issue -- as you
say, that replica will fail at START_REPLICATION time.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 2 May 2022 20:37:59 +0200

> problem in using different subscriptions for such cases?

I kept that example intentionally simple, but I'm sure we could come up
with more complex use cases. Following the "data redaction" idea, we
could also apply the "deny all" approach, and do something like this:

-- replicate the minimal column list by default (replica identity)
CREATE PUBLICATION p1 FOR TABLE t (id, region);

-- replicate more columns for the selected region
CREATE PUBLICATION p2 FOR TABLE t (...) WHERE (region = 'USA')

Now, I admit this is something I just made up, but I think it seems like
a pretty common approach.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 2 May 2022 20:40:08 +0200

> say, that replica will fail at START_REPLICATION time.

Right, I got confused a bit.

Anyway, I think the main challenge is defining what exactly we want to
check, in order to ensure "sensible" behavior, without preventing way
too many sensible use cases.

regards

---

From: Amit Kapila <[email-masked]>
Date: Tue, 3 May 2022 09:00:40 +0530

> too many sensible use cases.

I could think of below two options:
1. Forbid any case where column list is different for the same table
when combining publications.
2. Forbid if the column list and row filters for a table are different
in the set of publications we are planning to combine. This means we
will allow combining column lists when row filters are not present or
when column list is the same (we don't get anything additional by
combining but the idea is we won't forbid such cases) and row filters
are different.

Now, I think the points in favor of (1) are that the main purpose of
introducing a column list are: (a) the structure/schema of the
subscriber is different from the publisher, (b) want to hide sensitive
columns data. In both cases, it should be fine if we follow (1) and
from Peter E.'s latest email [1] he also seems to be indicating the
same. If we want to be slightly more relaxed then we can probably (2).
We can decide on something else as well but I feel it should be such
that it is easy to explain.

[1] - https://www.postgresql.org/message-id/47dd2cb9-4e96-169f-15ac-f9407fb54d43%40enterprisedb.com

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 3 May 2022 09:23:04 +0530

On Mon, May 2, 2022 at 6:11 PM Alvaro Herrera <[email-masked]> wrote:
> reading from it.  Is that what you have in mind?
>

Yes, something on these lines. We need a way to get the list of
publications each walsender is publishing data for.

>  Alternatively, we

I think for this we need to check DSM for all the publications and I
feel in general publications should be more than the number of
walsenders, so the previous approach seems better to me. However, any
one of these or similar ideas should be okay.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 6 May 2022 03:23:55 +0000

> that it is easy to explain.

I also think it makes sense to add a restriction like (1). I am planning to
implement the restriction if no one objects.

Best regards,
Hou zj

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 6 May 2022 14:26:27 +0200

> implement the restriction if no one objects.

I'm not going to block that approach if that's the consensus here,
though I'm not convinced.

Let me point out (1) does *not* work for data redaction use case,
certainly not the example Alvaro and me presented, because that relies
on a combination of row filters and column filters. Requiring all column
lists to be the same (and not specific to row filter) prevents that
example from working. Yes, you can create multiple subscriptions, but
that brings it's own set of challenges too.

I doubt forcing users to use the more complex setup is good idea, and
combining the column lists per [1] seems sound to me.

That being said, the good thing is this restriction seems it might be
relaxed in the future to work per [1], without causing any backwards
compatibility issues.

Should we do something similar for row filters, though? It seems quite
weird we're so concerned about unexpected behavior due to combining
column lists (despite having a patch that makes it behave sanely), and
at the same time wave off similarly strange behavior due to combining
row filters because "that's what you get if you define the publications
in a strange way".

regards

[1]
https://www.postgresql.org/message-id/5a85b8b7-fc1c-364b-5c62-0bb3e1e25824%40enterprisedb.com

---

From: Amit Kapila <[email-masked]>
Date: Fri, 6 May 2022 19:10:16 +0530

On Fri, May 6, 2022 at 5:56 PM Tomas Vondra
<[email-masked]> wrote:
> on a combination of row filters and column filters.
>

This should just forbid the case presented by Alvaro in his first
email in this thread [1].

> Requiring all column

These are my thoughts as well. Even, if we decide to go via the column
list merging approach (in selective cases), we need to do some
performance testing of that approach as it does much more work per
tuple. It is possible that the impact is not much but still worth
evaluating, so let's try to see the patch to prohibit combining the
column lists then we can decide.

> row filters because "that's what you get if you define the publications

During development, we found that we can't combine the row-filters for
'insert' and 'update'/'delete' because of replica identity
restrictions, so we have kept them separate. But if we came across
other such things then we can either try to fix those or forbid them.

[1] - https://www.postgresql.org/message-id/202204251548.mudq7jbqnh7r%40alvherre.pgsql

-- 
With Regards,
Amit Kapila.

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 6 May 2022 15:57:26 +0200

> column lists then we can decide.

Surely we could do some performance testing now. I doubt it's very
expensive - sure, you can construct cases with many row filters / column
lists, but how likely is that in practice?

Moreover, it's not like this would affect existing setups, so even if
it's a bit expensive, we may interpret that as cost of the feature.

> other such things then we can either try to fix those or forbid them.

I understand how we got to the current state. I'm just saying that this
allows defining separate publications for insert, update and delete
actions, and set different row filters for each of them. Which results
in behavior that is hard to explain/understand, especially when it comes
to tablesync.

It seems quite strange to prohibit merging column lists because there
might be some strange behavior that no one described, and allow setups
with different row filters that definitely have strange behavior.

regards

---

From: Amit Kapila <[email-masked]>
Date: Sat, 7 May 2022 11:06:02 +0530

> The latter might be better.

While thinking about using DSM here, I came across one of your commits
f2f9fcb303 which seems to indicate that it is not a good idea to rely
on it but I think you have changed dynamic shared memory to fixed
shared memory usage because that was more suitable rather than DSM is
not portable. Because I see a commit bcbd940806 where we have removed
the 'none' option of dynamic_shared_memory_type. So, I think it should
be okay to use DSM in this context. What do you think?

-- 
With Regards,
Amit Kapila.

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 8 May 2022 20:11:08 +0200

> be okay to use DSM in this context. What do you think?

Why would any of this be needed?

ALTER PUBLICATION will invalidate the RelationSyncEntry entries in all
walsenders, no? So AFAICS it should be enough to enforce the limitations
in get_rel_sync_entry, which is necessary anyway because the subscriber
may not be connected when ALTER PUBLICATION gets executed.

regards

---

From: Amit Kapila <[email-masked]>
Date: Mon, 9 May 2022 09:15:58 +0530

On Sun, May 8, 2022 at 11:41 PM Tomas Vondra
<[email-masked]> wrote:
> in get_rel_sync_entry,

Yes, that should be sufficient to enforce limitations in
get_rel_sync_entry() but it will lead to the following behavior:
a. The Alter Publication command will be successful but later in the
logs, the error will be logged and the user needs to check it and take
appropriate action. Till that time the walsender will be in an error
loop which means it will restart and again lead to the same error till
the user takes some action.
b. As we use historic snapshots, so even after the user takes action
say by changing publication, it won't be reflected. So, the option for
the user would be to drop their subscription.

Am, I missing something? If not, then are we okay with such behavior?
If yes, then I think it would be much easier implementation-wise and
probably advisable at this point. We can document it so that users are
careful and can take necessary action if they get into such a
situation. Any way we can improve this in future as you also suggested
earlier.

> which is necessary anyway because the subscriber

If we are not okay with the resultant behavior of detecting this in
get_rel_sync_entry(), then we can solve this in some other way as
Alvaro has indicated in one of his responses which is to detect that
at start replication time probably in the subscriber-side.

-- 
With Regards,
Amit Kapila.

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 10 May 2022 21:05:51 +0200

> at start replication time probably in the subscriber-side.

IMO that behavior is acceptable. We have to do that check anyway, and
the subscription may start failing after ALTER PUBLICATION for a number
of other reasons anyway so the user needs/should check the logs.

And if needed, we can improve this and start doing the proactive-checks
during ALTER PUBLICATION too.

regards

---

From: Amit Kapila <[email-masked]>
Date: Wed, 11 May 2022 09:03:18 +0530

On Wed, May 11, 2022 at 12:35 AM Tomas Vondra
<[email-masked]> wrote:
> IMO that behavior is acceptable.
>

Fair enough, then we should go with a simpler approach to detect it in
pgoutput.c (get_rel_sync_entry).

> We have to do that check anyway, and

I think ALTER PUBLICATION won't ever lead to failure in walsender.
Sure, users can do something due to which subscriber-side failures can
happen due to constraint failures. Do you have some specific cases in
mind?

> during ALTER PUBLICATION too.

Agreed.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 11 May 2022 07:25:03 +0000

On Wednesday, May 11, 2022 11:33 AM Amit Kapila <[email-masked]> wrote:
> > >>>>> LogicalDecodingContext->output_plugin_private. AFAIK, each
> > >>>>> LogicalDecodingContext->walsender
> > >>>>> can have one such LogicalDecodingContext and we can probably
> > >>>>> share it via shared memory?
> > >>>>
> > >>>> connection, at START_REPLICATION time.  Then ALTER PUBLICATION
> > >>>> needs to connect to all DSMs of all running walsenders and see if
> > >>>> they are reading from it.  Is that what you have in mind?
> > >>>> Alternatively, we could have one DSM per publication with a PID
> > >>>> array of all walsenders that are sending it (each walsender needs to
> add its PID as it starts).
> > >>> f2f9fcb303 which seems to indicate that it is not a good idea to
> > >>> rely on it but I think you have changed dynamic shared memory to
> > >>> fixed shared memory usage because that was more suitable rather
> > >>> than DSM is not portable. Because I see a commit bcbd940806 where
> > >>> we have removed the 'none' option of dynamic_shared_memory_type.
> > >>> So, I think it should be okay to use DSM in this context. What do you
> think?
> > >>>
> > >>
> > >> ALTER PUBLICATION will invalidate the RelationSyncEntry entries in
> > >> all walsenders, no? So AFAICS it should be enough to enforce the
> > >> limitations in get_rel_sync_entry,
> > >>
> > >
> > > logs, the error will be logged and the user needs to check it and
> > > take appropriate action. Till that time the walsender will be in an
> > > error loop which means it will restart and again lead to the same
> > > error till the user takes some action.
> > > say by changing publication, it won't be reflected. So, the option
> > > for the user would be to drop their subscription.
> > >
> > > probably advisable at this point. We can document it so that users
> > > are careful and can take necessary action if they get into such a
> > > situation. Any way we can improve this in future as you also
> > > suggested earlier.
> > >
> > >> which is necessary anyway because the subscriber may not be
> > >> connected when ALTER PUBLICATION gets executed.
> > >>
> > >

OK, here is the patch that try to check column list in that way. The patch also
check the column list when CREATE SUBSCRIPTION and when starting initial copy.

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Thu, 12 May 2022 12:15:24 +0530

> check the column list when CREATE SUBSCRIPTION and when starting initial copy.

Few comments:
===============
1.
initStringInfo(&cmd);
- appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename\n"
-    "  FROM pg_catalog.pg_publication_tables t\n"
+ appendStringInfoString(&cmd,
+    "SELECT DISTINCT t.schemaname,\n"
+    "                t.tablename,\n"
+    "                (CASE WHEN (array_length(pr.prattrs, 1) = t.relnatts)\n"
+    "                THEN NULL ELSE pr.prattrs END)\n"
+    "  FROM (SELECT P.pubname AS pubname,\n"
+    "               N.nspname AS schemaname,\n"
+    "               C.relname AS tablename,\n"
+    "               P.oid AS pubid,\n"
+    "               C.oid AS reloid,\n"
+    "               C.relnatts\n"
+    "          FROM pg_publication P,\n"
+    "          LATERAL pg_get_publication_tables(P.pubname) GPT,\n"
+    "          pg_class C JOIN pg_namespace N\n"
+    "                     ON (N.oid = C.relnamespace)\n"
+    "          WHERE C.oid = GPT.relid) t\n"
+    "  LEFT OUTER JOIN pg_publication_rel pr\n"
+    "       ON (t.pubid = pr.prpubid AND\n"
+    "        pr.prrelid = reloid)\n"

Can we modify pg_publication_tables to get the row filter and column
list and then use it directly instead of constructing this query?

2.
+ if (list_member(tablelist, rv))
+ ereport(WARNING,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use different column lists for table \"%s.%s\" in
different publications",
+    nspname, relname));
+ else

Can we write comments to explain why we are using WARNING here instead of ERROR?

3.
static void
-pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry)
+pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry,
+   Relation relation)

What is the need to change this interface as part of this patch?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 12 May 2022 14:02:02 +0530

> > check the column list when CREATE SUBSCRIPTION and when starting initial copy.
...

One more point, I think we should update the docs for this.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 13 May 2022 06:02:47 +0000

On Thursday, May 12, 2022 2:45 PM Amit Kapila <[email-masked]> wrote:
> > > Fair enough, then we should go with a simpler approach to detect it
> > > in pgoutput.c (get_rel_sync_entry).
> >
> > OK, here is the patch that try to check column list in that way. The
> > patch also check the column list when CREATE SUBSCRIPTION and when
> starting initial copy.
> >
> 
> Few comments:
> ===============
> 1.
> initStringInfo(&cmd);
> Can we modify pg_publication_tables to get the row filter and column list and
> then use it directly instead of constructing this query?

Agreed. If we can get columnlist and rowfilter from pg_publication_tables, it
will be more convenient. And I think users that want to fetch the columnlist
and rowfilter of table can also benefit from it.

> What is the need to change this interface as part of this patch?

Attach the new version patch which addressed these comments and update the
document. 0001 patch is to extent the view and 0002 patch is to add restriction
for column list.

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Mon, 16 May 2022 11:40:14 +0530

> and rowfilter of table can also benefit from it.

After the change for this, we will give an error on combining
publications where one of the publications specifies all columns in
the table and the other doesn't provide any columns. We should not
give an error as both mean all columns.

> document. 0001 patch is to extent the view and 0002 patch is to add restriction

Few  comments:
=================
1.
postgres=# select * from pg_publication_tables;
 pubname | schemaname | tablename | columnlist | rowfilter
---------+------------+-----------+------------+-----------
 pub1    | public     | t1        |            |
 pub2    | public     | t1        | 1 2        | (c3 < 10)
(2 rows)

I think it is better to display column names for columnlist in the
exposed view similar to attnames in the pg_stats_ext view. I think
that will make it easier for users to understand this information.

2.
 { oid => '6119', descr => 'get OIDs of tables in a publication',
   proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
-  provolatile => 's', prorettype => 'oid', proargtypes => 'text',
-  proallargtypes => '{text,oid}', proargmodes => '{i,o}',
-  proargnames => '{pubname,relid}', prosrc => 'pg_get_publication_tables' },
+  provolatile => 's', prorettype => 'record', proargtypes => 'text',
+  proallargtypes => '{text,oid,int2vector,pg_node_tree}', proargmodes
=> '{i,o,o,o}',

I think we should change the "descr" to something like: 'get
information of tables in a publication'

3.
+
+ /*
+ * We only throw a warning here so that the subcription can still be
+ * created and let user aware that something is going to fail later and
+ * they can fix the publications afterwards.
+ */
+ if (list_member(tablelist, rv))
+ ereport(WARNING,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use different column lists for table \"%s.%s\" in
different publications",
+    nspname, relname));

Can we extend this comment to explain the case where after Alter
Publication, if the user dumps and restores back the subscription,
there is a possibility that "CREATE SUBSCRIPTION" won't work if we
give ERROR here instead of WARNING?

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 16 May 2022 12:34:25 +0000

On Monday, May 16, 2022 2:10 PM Amit Kapila <[email-masked]>
> give an error as both mean all columns.

Thanks for the comments. Fixed.

> > document. 0001 patch is to extent the view and 0002 patch is to add

Agreed and changed.

> information of tables in a publication'

Changed.

> give ERROR here instead of WARNING?

After rethinking about this, It seems ok to report an ERROR here as the pg_dump
of subscription always set (connect = false). So, we won't hit the check when
restore the dump which means the restore can be successful even if user change
the publication afterwards. Based on this, I have changed the warning to error.

Attach the new version patch.

Best regards,
Hou zj

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 16 May 2022 15:20:33 +0200

> give an error as both mean all columns.

But don't we need to behave the same way for both column lists and row
filters?  I understand that some cases with different row filters for
different publications have shown to have weird behavior, so I think
it'd make sense to restrict it in the same way.  That would allow us to
extend the behavior in a sensible way when we develop that, instead of
setting in stone now behavior that we regret later.

> Few  comments:

+1

> information of tables in a publication'

+1

> give ERROR here instead of WARNING?

Yeah, and not only the comment — I think we need to have more in the
warning message itself.  How about:

ERROR:  cannot use different column lists for table "..." in different publications
DETAIL:  The subscription "..." cannot currently be used for replication.

I think this whole affair is a bit sad TBH and I'm sure it'll give us
some grief -- similar to replication slots becoming inactive and nobody
noticing.  A user changing a publication in a way that prevents some
replica from working and the warnings are hidden, they could have
trouble noticing that the replica is stuck.

But I have no better ideas.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 17 May 2022 03:25:29 +0000

> Attach the new version patch.

Thanks for your patch. Here are some comments:

1. (0001 patch)
/*
 * Returns Oids of tables in a publication.
 */
Datum
pg_get_publication_tables(PG_FUNCTION_ARGS)

Should we modify the comment of pg_get_publication_tables() to "Returns
information of tables in a publication"?

2. (0002 patch)

+	 * Note that we don't support the case where column list is different for
+	 * the same table when combining publications. But we still need to check
+	 * all the given publication-table mappings and report an error if any
+	 * publications have different column list.
 	 *
 	 * Multiple publications might have multiple column lists for this
 	 * relation.

I think it would be better if we swap the order of these two paragraphs. 

Regards,
Shi yu

---

From: Amit Kapila <[email-masked]>
Date: Tue, 17 May 2022 08:56:21 +0530

On Mon, May 16, 2022 at 6:50 PM Alvaro Herrera <[email-masked]> wrote:
> it'd make sense to restrict it in the same way.
>

I think the case where we are worried about row filter behavior is for
initial table sync where we ignore publication actions and that is
true with and without row filters. See email [1]. We are planning to
document that behavior as a separate patch. The idea we have used for
row filters is similar to what IBM DB2 [2] and Oracle [3] uses where
they allow combining filters with pub-action (operation (insert,
update, delete) in their case).

I think both column lists and row filters have a different purpose and
we shouldn't try to make them behave in the same way. The main purpose
of introducing a column list is to have statically different shapes on
publisher and subscriber or hide sensitive column data. In both cases,
it doesn't seem to make sense to combine column lists and we didn't
find any other database doing so. OTOH, for row filters, it makes
sense to combine filters for each pub-action as both IBM DB2 and
Oracle seems to be doing.

> warning message itself.

But as mentioned by Hou-San in his last email (pg_dump of subscription
always set (connect = false) which means it won't try to fetch column
list), I think we don't need to give a WARNING here, instead, we can
use ERROR. So, do we need the extra DETAIL (The subscription "..."
cannot currently be used for replication.) as that is implicit for the
ERROR case?

> trouble noticing that the replica is stuck.

I agree and it seems this is the best we can do for now.

[1] - https://www.postgresql.org/message-id/CAA4eK1L_98LF7Db4yFY1PhKKRzoT83xtN41jTS5X%2B8OeGrAkLw%40mail.gmail.com
[2] - https://www.ibm.com/docs/en/idr/11.4.0?topic=rows-log-record-variables
[3] - https://docs.oracle.com/en/cloud/paas/goldengate-cloud/gwuad/selecting-and-filtering-rows.html#GUID-11296A70-D953-4426-8EAA-37C2B4432446

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 17 May 2022 06:49:20 +0000

> Attach the new version patch.
Hi,

I have few minor comments.

For v2-0001.

(1) Unnecessary period at the end of column explanation

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>rowfilter</structfield> <type>text</type>
+      </para>
+      <para>
+       Expression for the table's publication qualifying condition.
+      </para></entry>
+     </row>

It seems when we write a simple noun to explain a column,
we don't need to put a period at the end of the explanation.
Kindly change
FROM:
"Expression for the table's publication qualifying condition."
TO:
"Expression for the table's publication qualifying condition"

For v2-0002.

(a) typo in the commit message

Kindly change
FROM:
"In both cases, it doesn't seems to make sense to combine column lists."
TO:
"In both cases, it doesn't seem to make sense to combine column lists."
or "In both cases, it doesn't make sense to combine column lists."

(b) fetch_table_list

+               if (list_member(tablelist, rv))
+                       ereport(ERROR,
+                                       errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                       errmsg("cannot use different column lists for table \"%s.%s\" in different publications",
+                                                  nspname, relname));

Kindly add tests for new error paths, when we add them.

Best Regards,
	Takamichi Osumi

---

From: Amit Kapila <[email-masked]>
Date: Tue, 17 May 2022 12:22:45 +0530

> Attach the new version patch.

Few minor comments:
==================
1.
+      <para>
+       Names of table columns included in the publication. This contains all
+       the columns of table when user didn't specify column list for the
+       table.
+      </para></entry>

Can we slightly change it to: "Names of table columns included in the
publication. This contains all the columns of the table when the user
didn't specify the column list for the table."

2. Below comments needs to be removed from tablesync.c as we don't
combine column lists after this patch.

 * For initial synchronization, column lists can be ignored in following
* cases:
*
* 1) one of the subscribed publications for the table hasn't specified
* any column list
*
* 2) one of the subscribed publications has puballtables set to true
*
* 3) one of the subscribed publications is declared as ALL TABLES IN
* SCHEMA that includes this relation

3.
Note that we don't support the case where column list is different for
+ * the same table when combining publications. But we still need to check
+ * all the given publication-table mappings and report an error if any
+ * publications have different column list.

Can we change this comment to: "Note that we don't support the case
where the column list is different for the same table when combining
publications. But one can later change the publication so we still
need to check all the given publication-table mappings and report an
error if any publications have a different column list."?

4. Can we add a test for different column lists if it is not already there?

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 17 May 2022 09:10:00 +0000

> Few minor comments:

Thanks for the comments.

Attach the new version patch which addressed all the above comments and
comments from Shi yu[1] and Osumi-san[2].

[1] https://www.postgresql.org/message-id/OSZPR01MB6310F32344884F9C12F45071FDCE9%40OSZPR01MB6310.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/message-id/TYCPR01MB83736AEC2493FCBB75CC7556EDCE9%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 May 2022 07:58:36 +0530

> comments from Shi yu[1] and Osumi-san[2].

Thanks, your first patch looks good to me. I'll commit that tomorrow
unless there are more comments on the same. The second one is also in
good shape but I would like to test it a bit more and also see if
others have any suggestions/objections on the same.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 19 May 2022 10:33:13 +0530

> On 2022-May-16, Amit Kapila wrote:

I have committed the first patch after fixing this part. It seems Tom
is not very happy doing this after beta-1 [1]. The reason we get this
information via this view (and underlying function) is that it
simplifies the queries on the subscriber-side as you can see in the
second patch. The query change is as below:
@@ -1761,17 +1762,18 @@ fetch_table_list(WalReceiverConn *wrconn, List
*publications)
  WalRcvExecResult *res;
  StringInfoData cmd;
  TupleTableSlot *slot;
- Oid tableRow[2] = {TEXTOID, TEXTOID};
+ Oid tableRow[3] = {TEXTOID, TEXTOID, NAMEARRAYOID};
  List    *tablelist = NIL;

  initStringInfo(&cmd);
- appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename\n"
+ appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename, \n"
+    "                      t.attnames\n"
     "  FROM pg_catalog.pg_publication_tables t\n"
     " WHERE t.pubname IN (");

Now, there is another way to change this query as well as done by
Hou-San in his first version [2] of the patch. The changed query with
that approach will be something like:
@@ -1761,17 +1762,34 @@ fetch_table_list(WalReceiverConn *wrconn, List
*publications)
  WalRcvExecResult *res;
  StringInfoData cmd;
  TupleTableSlot *slot;
- Oid tableRow[2] = {TEXTOID, TEXTOID};
+ Oid tableRow[3] = {TEXTOID, TEXTOID, INT2VECTOROID};
  List    *tablelist = NIL;

  initStringInfo(&cmd);

It appeared slightly complex and costly to me, so I have given the
suggestion to change it as we have now in the second patch as shown
above. Now, I can think of below ways to proceed here:

a. Revert the change in view (and underlying function) as done in
commit 0ff20288e1 and consider the alternate way (using a slightly
complex query) to fix. Then maybe for PG-16, we can simplify it by
changing the underlying function and view.
b. Proceed with the current approach of using a simplified query.

What do you think?

[1] - https://www.postgresql.org/message-id/91075.1652929852%40sss.pgh.pa.us
[2] - https://www.postgresql.org/message-id/OS0PR01MB5716A594C58DE4FFD1F8100B94C89%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 19 May 2022 07:07:24 -0500

> [1] - https://www.postgresql.org/message-id/91075.1652929852%40sss.pgh.pa.us

I think Tom's concern is that adding information to a view seems like adding a
feature that hadn't previously been contemplated.
(Catalog changes themselves are not prohibited during the beta period).

> changing the underlying function and view.

But, ISTM that it makes no sense to do it differently for v15 just to avoid the
appearance of adding a new feature, only to re-do it in 2 weeks for v16...
So (from a passive observer) +0.1 to keep the current patch.

I have some minor language fixes to that patch.

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d96c72e5310..82aa84e96e1 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9691,7 +9691,7 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
 
      <row>
       <entry><link linkend="view-pg-publication-tables"><structname>pg_publication_tables</structname></link></entry>
-      <entry>publications and information of their associated tables</entry>
+      <entry>publications and information about their associated tables</entry>
      </row>
 
      <row>
@@ -11635,7 +11635,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
 
   <para>
    The view <structname>pg_publication_tables</structname> provides
-   information about the mapping between publications and information of
+   information about the mapping between publications and information about
    tables they contain.  Unlike the underlying catalog
    <link linkend="catalog-pg-publication-rel"><structname>pg_publication_rel</structname></link>,
    this view expands publications defined as <literal>FOR ALL TABLES</literal>
@@ -11695,7 +11695,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
       </para>
       <para>
        Names of table columns included in the publication. This contains all
-       the columns of the table when the user didn't specify the column list
+       the columns of the table when the user didn't specify a column list
        for the table.
       </para></entry>
      </row>
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 8c7fca62de3..2f706f638ce 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1077,7 +1077,7 @@ get_publication_name(Oid pubid, bool missing_ok)
 }
 
 /*
- * Returns information of tables in a publication.
+ * Returns information about tables in a publication.
  */
 Datum
 pg_get_publication_tables(PG_FUNCTION_ARGS)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 87aa571a331..86f13293090 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11673,7 +11673,7 @@
   prosrc => 'pg_show_replication_origin_status' },
 
 # publications
-{ oid => '6119', descr => 'get information of tables in a publication',
+{ oid => '6119', descr => 'get information about tables in a publication',
   proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
   provolatile => 's', prorettype => 'record', proargtypes => 'text',
   proallargtypes => '{text,oid,int2vector,pg_node_tree}', proargmodes => '{i,o,o,o}',

---

From: Tom Lane <[email-masked]>
Date: Thu, 19 May 2022 10:24:46 -0400

> (Catalog changes themselves are not prohibited during the beta period).

It certainly smells like a new feature, but my concern was more around the
post-beta catalog change.  We do those only if really forced to, and the
explanation in the commit message didn't satisfy me as to why it was
necessary.  This explanation isn't much better --- if we're trying to
prohibit a certain class of publication definitions, what good does it do
to check that on the subscriber side?  Even more to the point, how can we
have a subscriber do that by relying on view columns that don't exist in
older versions?  I'm also quite concerned about anything that involves
subscribers examining row filter conditions; that sounds like a pretty
direct route to bugs involving unsolvability and the halting problem.

(But I've not read very much of this thread ... been a bit under the
weather the last couple weeks.  Maybe this actually is a sane solution.
It just doesn't sound like one at this level of detail.)

			regards, tom lane

---

From: Amit Kapila <[email-masked]>
Date: Fri, 20 May 2022 08:36:16 +0530

On Thu, May 19, 2022 at 7:54 PM Tom Lane <[email-masked]> wrote:
> to check that on the subscriber side?
>

It is required on the subscriber side because prohibition is only for
the cases where multiple publications are combined. We disallow the
cases where the column list is different for the same table when
combining publications. For example:

Publisher-side:
Create table tab(c1 int, c2 int);
Create Publication pub1 for table tab(c1);
Create Publication pub1 for table tab(c2);

Subscriber-side:
Create Subscription sub1 Connection 'dbname=postgres' Publication pub1, pub2;

We want to prohibit such cases. So, it would be better to check at the
time of 'Create Subscription' to validate such combinations and
prohibit them. To achieve that we extended the existing function
pg_get_publication_tables() and view pg_publication_tables to expose
the column list and verify such a combination. We primarily need
column list information for this prohibition but it appeared natural
to expose the row filter.

As mentioned in my previous email, we can fetch the required
information directly from system table pg_publication_rel and extend
the query in fetch_table_list to achieve the desired purpose but
extending the existing function/view for this appears to be a simpler
way.

>  Even more to the point, how can we
> older versions?

We need a version check like (if
(walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)) for that.

>  I'm also quite concerned about anything that involves

We examine only the column list for the purpose of this prohibition.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 24 May 2022 05:33:50 +0000

On Friday, May 20, 2022 11:06 AM Amit Kapila <[email-masked]> wrote:
> > have a subscriber do that by relying on view columns that don't exist
> > in older versions?
> >
> 

Thanks for pointing it out. Here is the new version patch which add this version check.

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 May 2022 15:19:53 +0530

> to expose the row filter.

I still feel that the current approach to extend the underlying
function and view is a better idea but if you and or others are not
convinced then we can try to achieve it by extending the existing
query on the subscriber side as mentioned in my previous email [1].
Kindly let me know your opinion?

[1] - https://www.postgresql.org/message-id/CAA4eK1KfL%3Dez5fKPB-0Nrgf7wiqN9bXP-YHHj2YH5utXAmjYug%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 26 May 2022 08:56:32 +0530

> Kindly let me know your opinion?

Unless someone has objections or thinks otherwise, I am planning to
proceed with the approach of extending the function/view (patch for
which is already committed) and using it to prohibit the combinations
of publications having different column lists as is done in the
currently proposed patch [1].

[1] - https://www.postgresql.org/message-id/OS0PR01MB5716AD7C0FE7386630BDBAAB94D79%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 27 May 2022 11:17:00 +0530

> Thanks for pointing it out. Here is the new version patch which add this version check.

I have added/edited a few comments and ran pgindent. The attached
looks good to me. I'll push this early next week unless there are more
comments/suggestions.

-- 
With Regards,
Amit Kapila.

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 27 May 2022 00:53:31 -0500

On Fri, May 27, 2022 at 11:17:00AM +0530, Amit Kapila wrote:
> On Tue, May 24, 2022 at 11:03 AM [email-masked] <[email-masked]> wrote:
> >

A minor doc review.
Note that I also sent some doc comments at [email-masked].

+      lists among publications in which case <command>ALTER PUBLICATION</command>
+      command will be successful but later the WalSender in publisher or the

COMMA in which

remove "command" ?

s/in publisher/on the publisher/

+   Subscription having several publications in which the same table has been
+   published with different column lists is not supported.

Either "Subscriptions having .. are not supported"; or,
"A subscription having .. is not supported".

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 27 May 2022 07:34:32 +0000

On Friday, May 27, 2022 1:54 PM Justin Pryzby <[email-masked]> wrote:
> +      command will be successful but later the WalSender in publisher
> + or the
> 
> COMMA in which
> 
> remove "command" ?
> 
> Either "Subscriptions having .. are not supported"; or, "A subscription having ..
> is not supported".

Thanks for the comments. Here is the new version patch set which fixes these.

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Thu, 2 Jun 2022 17:28:34 +0530

> Thanks for the comments. Here is the new version patch set which fixes these.

I have pushed the bug-fix patch. I'll look at the language
improvements patch next.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 6 Jun 2022 15:42:31 +1000

> I have pushed the bug-fix patch. I'll look at the language

I noticed the patch "0001-language-fixes-on-HEAD-from-Justin.patch" says:

@@ -11673,7 +11673,7 @@

 # publications

~~~

But, this grammar website [1] says:

What Does Of Mean
As defined by Cambridge dictionary Of is basically used “to show
possession, belonging, or origin”.

What Does About Mean
Similarly about primarily indicates ‘On the subject of; concerning’ as
defined by the Oxford dictionary. Or about in brief highlights some
fact ‘on the subject of, or connected with’

The main difference between of and about is that of implies a
possessive quality while about implies concerning or on the subject of
something.

~~~

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 7 Jun 2022 22:25:21 -0500

...
> From which I guess
> 
> 1. 'get information of tables in a publication' ~= 'get information
> belonging to tables in a publication'

But the information doesn't "belong to" the tables.

The information is "regarding" the tables (or "associated with" or "concerned
with" or "respecting" or "on the subject of" the tables).

I think my change is correct based on the grammar definition, as well as its
intuitive "feel".

-- 
Justin

---

From: Peter Smith <[email-masked]>
Date: Wed, 8 Jun 2022 15:35:05 +1000

> I think my change is correct based on the grammar definition, as well as its

Actually, I have no problem with this being worded either way. My
point was mostly to question if it was really worth changing it at
this time - e.g. I think there is a reluctance to change anything to
do with the catalogs during beta (even when a catversion bump may not
be required).

I agree that "about" seems better if the text said, "get information
about tables". But it does not say that - it says "get information
about tables in a publication" which I felt made a subtle difference.

e.g.1 "... on the subject of / concerned with tables."
- sounds like attributes about each table (col names, row filter etc)

versus

e.g.2 "... on the subject of / concerned with tables in a publication."
- sounds less like information PER table, and more like information
about the table membership of the publication.

~~

Any ambiguities can be eliminated if this text was just fixed to be
consistent with the wording of catalogs.sgml:
e.g. "publications and information about their associated tables"

But then this comes full circle back to my question if during beta is
a good time to be making such a change.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 13 Jun 2022 08:54:42 +0530

> e.g. "publications and information about their associated tables"

I don't know if this is better than the current text for this view:
'get information of tables in a publication' and unless we have a
consensus on any change here, I think it is better to retain the
current text as it is.

I would like to close the Open item listed corresponding to this
thread [1] as the fix for the reported issue is committed
(fd0b9dcebd). Do let me know if you or others think otherwise?

[1] - https://wiki.postgresql.org/wiki/PostgreSQL_15_Open_Items

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 16 Jun 2022 09:53:17 +0530

> (fd0b9dcebd). Do let me know if you or others think otherwise?

Seeing no objections, I have closed this item.

-- 
With Regards,
Amit Kapila.