Horton

Re: Table AM Interface Enhancements

Pavel Borisov <pashkin.elfe@gmail.com>

View thread View on postgr.es
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Nikita Malakhov <hukutoc@gmail.com>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-11-29T14:35:28Z
Lists: pgsql-hackers 
Hi, Nikita!

On Wed, 29 Nov 2023 at 18:27, Nikita Malakhov <hukutoc@gmail.com> wrote:

> Hi,
>
> Pavel, as far as I understand Alexander's idea assertion and especially
> ereport
> here does not make any sense - this method is not considered to report
> error, it
> silently calls if there is underlying [free] function and simply falls
> through otherwise,
> also, take into account that it could be located in the uninterruptible
> part of the code.
>
> On the whole topic I have to
>
> On Wed, Nov 29, 2023 at 4:56 PM Pavel Borisov <pashkin.elfe@gmail.com>
> wrote:
>
>> Hi, Alexander!
>>
>>> I'm planning to review some of the other patches from the current
>>> patchset soon.
>>>
>>
>> I've looked into the patch 0003.
>> The patch looks in good shape and is uncontroversial to me. Making memory
>> structures to be dynamically allocated is simple enough and it allows to
>> store complex data like lists etc. I consider places like this that expect
>> memory structures to be flat and allocated at once are because the was no
>> need in more complex ones previously. If there is a need for them, I think
>> they could be added without much doubt, provided the simplicity of the
>> change.
>>
>> For the code:
>> +static inline void
>> +table_free_rd_amcache(Relation rel)
>> +{
>> + if (rel->rd_tableam && rel->rd_tableam->free_rd_amcache)
>> + {
>> + rel->rd_tableam->free_rd_amcache(rel);
>> + }
>> + else
>> + {
>> + if (rel->rd_amcache)
>> + pfree(rel->rd_amcache);
>> + rel->rd_amcache = NULL;
>> + }
>>
>> here I suggest adding Assert(rel->rd_amcache == NULL) (or maybe better an
>> error report) after calling free_rd_amcache to be sure the custom
>> implementation has done what it should do.
>>
>> Also, I think some brief documentation about writing this custom method
>> is quite relevant maybe based on already existing comments in the code.
>>
>> Kind regards,
>> Pavel
>>
>
> When we do default single chunk routine we invalidate rd_amcache pointer,
+ if (rel->rd_amcache)
+ pfree(rel->rd_amcache);
+ rel->rd_amcache = NULL;

If we delegate this to method, my idea is to check the method
implementation don't leave this pointer valid.
If it's not needed, I'm ok with it, but to me it seems that the check I
proposed makes sense.

Regards,
Pavel

Commits

  1. Remove extra comment at TableAmRoutine.scan_analyze_next_block

  2. revert: Generalize relation analyze in table AM interface

  3. Revert: Allow table AM to store complex data structures in rd_amcache

  4. Revert: Allow table AM tuple_insert() method to return the different slot

  5. Revert: Allow locking updated tuples in tuple_update() and tuple_delete()

  6. Revert: Let table AM insertion methods control index insertion

  7. Revert: Custom reloptions for table AM

  8. Provide a way block-level table AMs could re-use acquire_sample_rows()

  9. Custom reloptions for table AM

  10. Use streaming I/O in ANALYZE.

  11. Revert "Custom reloptions for table AM"

  12. Let table AM insertion methods control index insertion

  13. Generalize relation analyze in table AM interface

  14. Improve error message for tts_(virtual|minimal)_is_current_xact_tuple

  15. Add comments on some MinimalTupleSlots methods usage

  16. Add TupleTableSlotOps.is_current_xact_tuple() method

  17. Allow table AM tuple_insert() method to return the different slot

  18. Allow table AM to store complex data structures in rd_amcache