From: | "Drouvot, Bertrand" <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>, fabriziomello(at)gmail(dot)com, tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>, Rahila Syed <rahila(dot)syed(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Minimal logical decoding on standbys |
Date: | 2022-12-15 10:31:42 |
Message-ID: | c032a40f-c294-549b-1686-672c172c6074@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2022-12-15 11:07:19 | Re: plpgsq_plugin's stmt_end() is not called when an error is caught |
Previous Message | Drouvot, Bertrand | 2022-12-15 10:20:18 | Re: Minimal logical decoding on standbys |