Re: Minimal logical decoding on standbys

Hi,

Just realized that Melanie was missing in the up-thread reply to
her feedback (not sure what happened, sorry about that).... So, adding her here.

Please find attached V45 addressing Melanie's feedback.

On 1/24/23 3:59 PM, Drouvot, Bertrand wrote:
> Hi,
> 
> On 1/24/23 12:21 AM, Melanie Plageman wrote:
>> I'm new to this thread and subject, but I had a few basic thoughts about
>> the first patch in the set.
>>
> 
> Thanks for looking at it!
> 
>> On Mon, Jan 23, 2023 at 12:03:35PM +0100, Drouvot, Bertrand wrote:
>>> 1. We want to enable logical decoding on standbys, but replay of WAL
>>> from the primary might remove data that is needed by logical decoding,
>>> causing replication conflicts much as hot standby does.
>>
>> It is a little confusing to mention replication conflicts in point 1. It
>> makes it sound like it already logs a recovery conflict. Without the
>> recovery conflict handling in this patchset, logical decoding of
>> statements using data that has been removed will fail with some error
>> like :
>>     ERROR:  could not map filenumber "xxx" to relation OID
>> Part of what this patchset does is introduce the concept of a new kind
>> of recovery conflict and a resolution process.

Changed the wording in V45's commit message.

>>> 3. To do this we need the latestRemovedXid for each change, just as we
>>> do for physical replication conflicts, but we also need to know
>>> whether any particular change was to data that logical replication
>>> might access.
> 
>> It isn't clear from the above sentence why you would need both. I think
>> it has something to do with what is below (hot_standby_feedback being
>> off), but I'm not sure, so the order is confusing.
> 

Trying to be more clear in the new commit message.

> 
>>> Implementation:
>>>
>>> When a WAL replay on standby indicates that a catalog table tuple is
>>> to be deleted by an xid that is greater than a logical slot's
>>> catalog_xmin, then that means the slot's catalog_xmin conflicts with
>>> the xid, and we need to handle the conflict. While subsequent commits
>>> will do the actual conflict handling, this commit adds a new field
>>> isCatalogRel in such WAL records (and a new bit set in the
>>> xl_heap_visible flags field), that is true for catalog tables, so as to
>>> arrange for conflict handling.
>>
>> You do mention it a bit here, but I think it could be more clear and
>> specific.
> 

Added the WAL record types impacted by the change in the new commit message.


>>
>> It is not very important, but I noticed you made "heaprel" the last
>> parameter to all of the btree-related functions but the first parameter
>> to the gist functions. I thought it might be nice to make the order
>> consistent.
> 
> Agree, will do.

Done.

> 
>> I also was wondering why you made it the last argument to
>> all the btree functions to begin with (i.e. instead of directly after
>> the first rel argument).
>>
> 
> No real reasons, will put all of them after the first rel argument (that seems a better place).

Done.

> 
>>> diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
>>> index 8af33d7b40..9bdac12baf 100644
>>> --- a/src/include/access/gist_private.h
>>> +++ b/src/include/access/gist_private.h
>>> @@ -440,7 +440,7 @@ extern XLogRecPtr gistXLogPageDelete(Buffer buffer,
>>>                                        FullTransactionId xid, Buffer parentBuffer,
>>>                                        OffsetNumber downlinkOffset);
>>> -extern void gistXLogPageReuse(Relation rel, BlockNumber blkno,
>>> +extern void gistXLogPageReuse(Relation heaprel, Relation rel, BlockNumber blkno,
>>>                                 FullTransactionId deleteXid);
>>>   extern XLogRecPtr gistXLogUpdate(Buffer buffer,
>>> @@ -485,7 +485,7 @@ extern bool gistproperty(Oid index_oid, int attno,
>>>   extern bool gistfitpage(IndexTuple *itvec, int len);
>>>   extern bool gistnospace(Page page, IndexTuple *itvec, int len, OffsetNumber todelete, Size freespace);
>>>   extern void gistcheckpage(Relation rel, Buffer buf);
>>> -extern Buffer gistNewBuffer(Relation r);
>>> +extern Buffer gistNewBuffer(Relation heaprel, Relation r);
>>>   extern bool gistPageRecyclable(Page page);
>>>   extern void gistfillbuffer(Page page, IndexTuple *itup, int len,
>>>                              OffsetNumber off);
>>> diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
>>> index 09f9b0f8c6..191f0e5808 100644
>>> --- a/src/include/access/gistxlog.h
>>> +++ b/src/include/access/gistxlog.h
>>> @@ -51,13 +51,13 @@ typedef struct gistxlogDelete
>>>   {
>>>       TransactionId snapshotConflictHorizon;
>>>       uint16        ntodelete;        /* number of deleted offsets */
>>> +    bool        isCatalogRel;
>>
>> In some of these struct definitions, I think it would help comprehension
>> to have a comment explaining the purpose of this member.
>>
> 
> Yeah, agree but it could be done in another patch (outside of this feature), agree?

Please forget about my previous reply (I misunderstood and thought you were mentioning the offset's Array).

Added comments about isCatalogRel in V45 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com