Re: Minimal logical decoding on standbys

Hi,

On 9/30/22 2:11 PM, Drouvot, Bertrand wrote:
> Hi,
>
> On 7/6/22 3:30 PM, Drouvot, Bertrand wrote:
>> Hi,
>>
>> On 10/28/21 11:07 PM, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2021-10-28 16:24:22 -0400, Robert Haas wrote:
>>>> On Wed, Oct 27, 2021 at 2:56 AM Drouvot, Bertrand <bdrouvot(at)amazon(dot)com> wrote:
>>>>> So you have in mind to check for XLogLogicalInfoActive() first, and if true, then open the relation and call
>>>>> RelationIsAccessibleInLogicalDecoding()?
>>>> I think 0001 is utterly unacceptable. We cannot add calls to
>>>> table_open() in low-level functions like this. Suppose for example
>>>> that _bt_getbuf() calls _bt_log_reuse_page() which with 0001 applied
>>>> would call get_rel_logical_catalog(). _bt_getbuf() will have acquired
>>>> a buffer lock on the page. The idea that it's safe to call
>>>> table_open() while holding a buffer lock cannot be taken seriously.
>>> Yes - that's pretty clearly a deadlock hazard. It shouldn't too hard to fix, I
>>> think. Possibly a bit more verbose than nice, but...
>>>
>>> Alternatively we could propagate the information whether a relcache entry is
>>> for a catalog from the table to the index. Then we'd not need to change the
>>> btree code to pass the table down.
>>
>> Looking closer at RelationIsAccessibleInLogicalDecoding() It seems to me that the missing part to be able to tell whether or not an index is for a catalog is the rd_options->user_catalog_table value of its related heap relation.
>>
>> Then, a way to achieve that could be to:
>>
>> - Add to Relation a new "heap_rd_options" representing the rd_options of the related heap relation when appropriate
>>
>> - Trigger the related indexes relcache invalidations when an ATExecSetRelOptions() is triggered on a heap relation
>>
>> - Write an equivalent of RelationIsUsedAsCatalogTable() for indexes that would make use of the heap_rd_options instead
>>
>> Does that sound like a valid option to you or do you have another idea in mind to propagate the information whether a relcache entry is for a catalog from the table to the index?
>>
>
> I ended up with the attached proposal to propagate the catalog information to the indexes.
>
> The attached adds a new field "isusercatalog" in pg_index to indicate whether or not the index is linked to a table that has the storage parameter user_catalog_table set to true.
>
> Then it defines new macros, including "IndexIsAccessibleInLogicalDecoding" making use of this new field.
>
> This new macro replaces get_rel_logical_catalog() that was part of the previous patch version.
>
> What do you think about this approach and the attached?
>
> If that sounds reasonable, then I'll add tap tests for it and try to improve the way isusercatalog is propagated to the index(es) in case a reset is done on user_catalog_table on the table (currently in this POC patch, it's hardcoded to "false" which is the default value for user_catalog_table in boolRelOpts[]) (A better approach would be probably to retrieve the value from the table once the reset is done and then propagate it to the index(es).)

Please find attached a rebase to propagate the catalog information to the indexes.
It also takes care of the RESET on user_catalog_table (adding a new Macro "HEAP_DEFAULT_USER_CATALOG_TABLE") and adds a few tests in contrib/test_decoding/sql/ddl.sql.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com