Message-ID: <c032a40f-c294-549b-1686-672c172c6074@gmail.com>
Date: Thu, 15 Dec 2022 11:31:42 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: Minimal logical decoding on standbys
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 Ibrar Ahmed <ibrar.ahmad@gmail.com>, Amit Khandekar
 <amitdkhan.pg@gmail.com>, fabriziomello@gmail.com,
 tushar <tushar.ahuja@enterprisedb.com>,
 Rahila Syed <rahila.syed@2ndquadrant.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <b8cfc33a-36f0-a247-1c61-13500ef5ab0a@amazon.com>
 <688cbedf-a172-f0d8-8352-d4ea223bceeb@amazon.com>
 <CA+TgmobgOLH-JpBoBSdu4i+sjRdgwmDEZGECkmowXqQgQL6WhQ@mail.gmail.com>
 <20211028210755.afmwcvpo6ajwdx6n@alap3.anarazel.de>
 <6711d09f-eaa9-264b-ec7f-88004218b075@amazon.com>
 <178cf7da-9bd7-e328-9c49-e28ac4701352@gmail.com>
 <ad9e7cb2-92ac-04b8-537c-c325b358e2c6@gmail.com>
 <b0366278-a48a-1af2-c389-e84c1e207d06@gmail.com>
 <4b19419f-2e7c-a76b-c948-26d10af6693d@gmail.com>
 <20221207175824.apmc5clnj2fdjkaq@awork3.anarazel.de>
 <fe81c2e4-aeca-4e57-b453-f183531ce389@gmail.com>
 <09c26df0-c5f9-eae0-5a6a-10fb00b0522e@gmail.com>
 <CA+TgmoY0df9X+5ENg8P0BGj0odhM45sdQ7kB4JMo4NKaoFy-Vg@mail.gmail.com>
 <9f0a021e-40e1-0ae6-8086-a4457c99749e@gmail.com>
 <CA+Tgmoa1cZSo1tUTzxS+RyniMuNtkpxUeEhnD2+-mvJodVtdSQ@mail.gmail.com>
 <0ae83e69-bda0-0eb8-ef4a-d169292e3f66@gmail.com>
 <47f5faf0-6a6d-602a-5d5b-9449ca74c6ad@gmail.com>
 <CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: 
 <CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c032a40f-c294-549b-1686-672c172c6074%40gmail.com>
Precedence: bulk

Hi,

On 12/14/22 4:55 PM, Robert Haas wrote:
>   On Wed, Dec 14, 2022 at 8:06 AM Drouvot, Bertrand> 
> Other comments:
> 
> +    if RelationIsAccessibleInLogicalDecoding(rel)
> +        xlrec.flags |= VISIBILITYMAP_ON_CATALOG_ACCESSIBLE_IN_LOGICAL_DECODING;
> 
> This is a few parentheses short of where it should be. Hilariously it
> still compiles because there are parentheses in the macro definition.

Oops, thanks will fix.

> 
> +            xlrec.onCatalogAccessibleInLogicalDecoding =
> RelationIsAccessibleInLogicalDecoding(relation);
> 
> These lines are quite long. I think we should consider (1) picking a
> shorter name for the xlrec field and, if it's such lines are going to
> still routinely exceed 80 characters, (2) splitting them into two
> lines, with the second one indented to match pgindent's preferences in
> such cases, which I think is something like this:
> 
> xlrec.onCatalogAccessibleInLogicalDecoding =
>      RelationIsAccessibleInLogicalDecoding(relation);
> 
> As far as renaming, I think we could at least remove onCatalog part
> from the identifier, as that doesn't seem to be adding much. And maybe
> we could even think of changing it to something like
> logicalDecodingConflict or even decodingConflict, which would shave
> off a bunch more characters.


I'm not sure I like the decodingConflict proposal. Indeed, it might be there is no conflict (depending of the xids
comparison).

What about "checkForConflict"?

> 
> +    if (heapRelation->rd_options)
> +        isusercatalog = ((StdRdOptions *)
> (heapRelation)->rd_options)->user_catalog_table;
>
> Couldn't you get rid of the if statement here and also the
> initialization at the top of the function and just write isusercatalog
> = RelationIsUsedAsCatalogTable(heapRelation)? Or even just get rid of
> the variable entirely and pass
> RelationIsUsedAsCatalogTable(heapRelation) as the argument to
> UpdateIndexRelation directly?
> 

Yeah, that's better, will do, thanks!

While at it, I'm not sure that isusercatalog should be visible in pg_index.
I mean, this information could be retrieved with a join on pg_class (on the table the index is linked to), so the weirdness to have it visible.
I did not check how difficult it would be to make it "invisible" though.
What do you think?

> I think this could use some test cases demonstrating that
> indisusercatalog gets set correctly in all the relevant cases: table
> is created with user_catalog_table = true/false, reloption is changed,
> reloptions are reset, new index is added later, etc.
> 

v31 already provides a few checks:

- After index creation on relation with user_catalog_table = true
- Propagation is done correctly after a user_catalog_table RESET
- Propagation is done correctly after an ALTER SET user_catalog_table = true
- Propagation is done correctly after an ALTER SET user_catalog_table = false

In v32, I can add a check for index creation after each of the last 3 mentioned above and one when a table is created with user_catalog_table = false.

Having said that, we would need a function to retrieve the isusercatalog value should we make it invisible.

Regards,

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