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
