Re: Logical Replication - behavior of ALTER PUBLICATION .. DROP TABLE and ALTER SUBSCRIPTION .. REFRESH PUBLICATION

On Jan 14, 2021, at 1:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com<mailto:amit(dot)kapila16(at)gmail(dot)com>> wrote:

On Wed, Jan 13, 2021 at 5:40 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com<mailto:bharath(dot)rupireddyforpostgres(at)gmail(dot)com>> wrote:

On Wed, Jan 13, 2021 at 3:16 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com<mailto:bharath(dot)rupireddyforpostgres(at)gmail(dot)com>> wrote:

On Wed, Jan 13, 2021 at 2:36 PM japin <japinli(at)hotmail(dot)com<mailto:japinli(at)hotmail(dot)com>> wrote:
In summary, I feel we need to fix the publisher sending the inserts
even though the table is dropped from the publication, that is the
patch patch proposed by japin. This solves the bug reported in this
thread.

And also, it's good to have the LogicalRepRelMap invalidation fix as a
0002 patch in invalidate_syncing_table_states, subscription_change_cb
and logicalrep_rel_open as proposed by me.

Thoughts?

I think invalidate the LogicalRepRelMap is necessary. If the table isn't in
subscription, can we remove the LogicalRepRelMapEntry from LogicalRepRelMap?

IIUC, it's not possible to know the relid of the alter
publication...dropped table in the invalidation callbacks
invalidate_syncing_table_states or subscription_change_cb, so it's
better to set state of all the entries to SUBREL_STATE_UNKNOWN, so
that, in the next logicalrep_rel_open call the function
GetSubscriptionRelState will be called and the pg_subscription_rel
will be read freshly.

This is inline with the reasoning specified at [1].

[1] - https://www.postgresql.org/message-id/CAFiTN-udwuc_h0TaFrSEKb-Yo1vVvkjz9TDRw7VE3P-KiPSGbQ%40mail.gmail.com

Attaching following two patches:

0001 - Makes publisher to not publish the changes for the alter
publication ... dropped tables. Original patch is by japin, I added
comments, changed the function name and adjusted the code a bit.

Do we really need to access PUBLICATIONRELMAP in this patch? What if
we just set it to false in the else condition of (if (publish &&
(relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)))

Thank for you review. I agree with you, it doesn’t need to access PUBLICATIONRELMAP, since
We already get the publication oid in GetRelationPublications(relid) [1], which also access
PUBLICATIONRELMAP. If the current pub->oid does not in pubids, the publish is false, so we
do not need publish the table.

I have another question, the data->publications is a list, when did it has more then one items?

0001 - change as you suggest.
0002 - does not change.

Please consider v2 for further review.

[1]
List *
GetRelationPublications(Oid relid)
{
List *result = NIL;
CatCList *pubrellist;
int i;

/* Find all publications associated with the relation. */
pubrellist = SearchSysCacheList1(PUBLICATIONRELMAP,
ObjectIdGetDatum(relid));
for (i = 0; i < pubrellist->n_members; i++)
{
HeapTuple tup = &pubrellist->members[i]->tuple;
Oid pubid = ((Form_pg_publication_rel) GETSTRUCT(tup))->prpubid;

result = lappend_oid(result, pubid);
}

ReleaseSysCacheList(pubrellist);

return result;
}

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.