Re: Minimal logical decoding on standbys

Hi,

On 12/22/22 8:50 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 12/21/22 10:06 AM, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 12/20/22 10:41 PM, Robert Haas wrote:
>>> On Tue, Dec 20, 2022 at 3:39 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I guess whatever else we
>>> do here, we should fix the comments.
>>>
>>
>> Agree, please find attached a patch proposal doing so.
>>
>>
>>> Bottom line is that I think the two cases that have alignment issues
>>> as coded are xl_hash_vacuum_one_page and gistxlogDelete. Everything
>>> else is OK, as far as I can tell right now.
>>>
>>
>> Thanks a lot for the repro(s) and explanations! That's very useful/helpful.
>>
>> Based on your discovery about the wrong comments above, I'm now tempted to fix those 2 alignment issues
>> by using a FLEXIBLE_ARRAY_MEMBER within those structs (as you proposed in [1]) (as that should also prevent
>> any possible wrong comments about where the array is located).
>>
>> What do you think?
>
> As mentioned above, It looks to me that making use of a FLEXIBLE_ARRAY_MEMBER is a good choice.
> So, please find attached v35 making use of a FLEXIBLE_ARRAY_MEMBER in xl_hash_vacuum_one_page and gistxlogDelete (your 2 repros are not failing anymore).
> I've also added a few words in the commit message in 0001 about it.
>
> So, we end up with:
>
> (gdb) ptype /o struct xl_hash_vacuum_one_page
> /* offset      |    size */  type = struct xl_hash_vacuum_one_page {
> /*      0      |       4 */    TransactionId snapshotConflictHorizon;
> /*      4      |       4 */    int ntuples;
> /*      8      |       1 */    _Bool isCatalogRel;
> /* XXX  1-byte hole      */
> /*     10      |       0 */    OffsetNumber offsets[];
> /* XXX  2-byte padding   */
>
>                                /* total size (bytes):   12 */
>                              }
>
> (gdb) ptype /o struct gistxlogDelete
> /* offset      |    size */  type = struct gistxlogDelete {
> /*      0      |       4 */    TransactionId snapshotConflictHorizon;
> /*      4      |       2 */    uint16 ntodelete;
> /*      6      |       1 */    _Bool isCatalogRel;
> /* XXX  1-byte hole      */
> /*      8      |       0 */    OffsetNumber offsets[];
>
>                                /* total size (bytes):    8 */
>                              }
>
> While looking at it, I've a question: xl_hash_vacuum_one_page.ntuples is an int, do you see any reason why it is not an uint16? (we would get rid of 4 bytes in the struct).
>

Please find attached v36, tiny rebase due to 1de58df4fe.

Regards,

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