Hi,
On 12/15/22 11:31 AM, Drouvot, Bertrand wrote:
> 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.
Fixed in v32 attached.
>
>>
>> + 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"?
In v32 attached, it is renamed to mayConflictInLogicalDecoding (I think it's important it reflects
that it is linked to the logical decoding and the "uncertainty" of the conflict). What do you think?
>
>>
>> + 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!
Fixed in v32 attached.
>
> 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
weirdnessto have it visible.
> I did not check how difficult it would be to make it "invisible" though.
> What do you think?
>
It's still visible in v32 attached.
I had a second thought on it and it does not seem like a "real" concern to me.
>> 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
withuser_catalog_table = false.
>
v32 attached is adding the checks mentioned above.
v32 does not change anything linked to the alignment discussion, as I think this will depend of its outcome.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
