Re: Minimal logical decoding on standbys

Hi,

On 3/31/23 6:33 AM, Andres Freund wrote:
> Hi,
> 
> On 2023-03-30 18:23:41 +0200, Drouvot, Bertrand wrote:
>> On 3/30/23 9:04 AM, Andres Freund wrote:
>>> I think this commit is ready to go. Unless somebody thinks differently, I
>>> think I might push it tomorrow.
>>
>> Great! Once done, I'll submit a new patch so that GlobalVisTestFor() can make
>> use of the heap relation in vacuumRedirectAndPlaceholder() (which will be possible
>> once 0001 is committed).
> 
> Unfortunately I did find an issue doing a pre-commit review of the patch.
> 
> The patch adds VISIBILITYMAP_IS_CATALOG_REL to xl_heap_visible.flags - but it
> does not remove the bit before calling visibilitymap_set().
> 
> This ends up corrupting the visibilitymap, because the we'll set a bit for
> another page.
> 

Oh I see, I did not think about that (not enough experience in the VM area).
Nice catch and thanks for pointing out!

> On a casual read, one very well might think that VISIBILITYMAP_IS_CATALOG_REL
> is a valid bit that could be set in the VM.
> 

I see what you're saying now and do agree that's confusing.

> I am thinking of instead creating a separate namespace for the "xlog only"
> bits:
> 
> /*
>   * To detect recovery conflicts during logical decoding on a standby, we need
>   * to know if a table is a user catalog table. For that we add an additional
>   * bit into xl_heap_visible.flags, in addition to the above.
>   *
>   * NB: VISIBILITYMAP_XLOG_* may not be passed to visibilitymap_set().
>   */
> #define VISIBILITYMAP_XLOG_CATALOG_REL    0x04
> #define VISIBILITYMAP_XLOG_VALID_BITS    (VISIBILITYMAP_VALID_BITS | VISIBILITYMAP_XLOG_CATALOG_REL)
> 
> 
> That allows heap_xlog_visible() to do:
> 
>         Assert((xlrec->flags & VISIBILITYMAP_XLOG_VALID_BITS) == xlrec->flags);
>         vmbits = (xlrec->flags & VISIBILITYMAP_VALID_BITS);
> 
> and pass vmbits istead of xlrec->flags to visibilitymap_set().
> 

That sounds good to me. That way you'd ensure that VISIBILITYMAP_XLOG_CATALOG_REL is not
passed to visibilitymap_set().

> 
> I'm also thinking of splitting the patch into two. One patch to pass down the
> heap relation into the new places, and another for the rest.

I think that makes sense. I don't know how far you've work on the split but please
find attached V54 doing such a split + implementing your VISIBILITYMAP_XLOG_VALID_BITS
suggestion.

> 
> Note that gistXLogDelete() continues to register data with two different
> XLogRegisterData() calls. This will append data without any padding:
> 
>     XLogRegisterData((char *) &xlrec, SizeOfGistxlogDelete);
> 
>     /*
>      * We need the target-offsets array whether or not we store the whole
>      * buffer, to allow us to find the snapshotConflictHorizon on a standby
>      * server.
>      */
>     XLogRegisterData((char *) todelete, ntodelete * sizeof(OffsetNumber));
> 
> 
> But replay now uses the new offset member:
> 
>> @@ -177,6 +177,7 @@ gistRedoDeleteRecord(XLogReaderState *record)
>>       gistxlogDelete *xldata = (gistxlogDelete *) XLogRecGetData(record);
>>       Buffer        buffer;
>>       Page        page;
>> +    OffsetNumber *toDelete = xldata->offsets;
>>
>>       /*
>>        * If we have any conflict processing to do, it must happen before we
> 
> 
> That doesn't look right. If there's any padding before offsets, we'll afaict
> read completely bogus data?
> 
> As it turns out, there is padding:
> 
> struct gistxlogDelete {
>          TransactionId              snapshotConflictHorizon; /*     0     4 */
>          uint16                     ntodelete;            /*     4     2 */
>          _Bool                      isCatalogRel;         /*     6     1 */
> 
>          /* XXX 1 byte hole, try to pack */
> 
>          OffsetNumber               offsets[];            /*     8     0 */
> 
>          /* size: 8, cachelines: 1, members: 4 */
>          /* sum members: 7, holes: 1, sum holes: 1 */
>          /* last cacheline: 8 bytes */
> };
> 
> 
> I am frankly baffled how this works at all, this should just about immediately
> crash?
> 
> 

Oh, I see. Hm, don't we have already the same issue for spgxlogVacuumRoot / vacuumLeafRoot() / spgRedoVacuumRoot()?

> 
> I'm not going to commit a nontrivial change to these WAL records without some
> minimal tests.
> 

That makes fully sense.

Regards,

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