Re: Minimal logical decoding on standbys

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