public inbox for [email protected]  

help / color / mirror / Atom feed

From: Drouvot, Bertrand <[email protected]>
To: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Ibrar Ahmed <[email protected]>
Cc: Amit Khandekar <[email protected]>
Cc: [email protected]
Cc: tushar <[email protected]>
Cc: Rahila Syed <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Minimal logical decoding on standbys
Date: Thu, 15 Dec 2022 11:31:42 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CA+TgmobgOLH-JpBoBSdu4i+sjRdgwmDEZGECkmowXqQgQL6WhQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CA+TgmoY0df9X+5ENg8P0BGj0odhM45sdQ7kB4JMo4NKaoFy-Vg@mail.gmail.com>
	<[email protected]>
	<CA+Tgmoa1cZSo1tUTzxS+RyniMuNtkpxUeEhnD2+-mvJodVtdSQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CA+TgmoaVcu_mbxbH=EccvKG6u8+MdQf9zx98uAL9zsStFwrYsQ@mail.gmail.com>

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

view thread (196+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Minimal logical decoding on standbys
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox