public inbox for [email protected]
help / color / mirror / Atom feedFrom: Pavel Borisov <[email protected]>
To: Alexander Korotkov <[email protected]>
Cc: Japin Li <[email protected]>
Cc: Mark Dilger <[email protected]>
Cc: [email protected]
Subject: Re: Table AM Interface Enhancements
Date: Wed, 27 Mar 2024 16:51:54 +0400
Message-ID: <CALT9ZEFb0yVmZ_OQACFhYDEP3OSo4BPAMk9kju2Ap274D6SqJg@mail.gmail.com> (raw)
In-Reply-To: <CAPpHfdv6fDMkVZyJKuoaYdg5+zVnRNU6dMRM9m4mBH6+4W89ig@mail.gmail.com>
References: <CAPpHfdurb9ycV8udYqM=o0sPS66PJ4RCBM1g-bBpvzUfogY0EA@mail.gmail.com>
<[email protected]>
<CAPpHfdv0g30VPKBwK=jCEmDdybgdu2Ro8VSNbLnc22C6qKsaZA@mail.gmail.com>
<[email protected]>
<CAPpHfduNh1iTo1KGBLAWBBt6MZQ00YkSVvUPgFDp6vUKY5Qo8w@mail.gmail.com>
<CAPpHfduey2T2jebBzUiVAVivyNjRQ22EH_K=F6VCozB=f13tzQ@mail.gmail.com>
<CALT9ZEF5i_bhrtUhM4bR5uFFk-aCXsWYAWSrv0FAELXuFYk2Lw@mail.gmail.com>
<CAPpHfdvkessZ_Nj8O6o+PxfrZjVpESkhcBKT1HHjMezB1_zFWg@mail.gmail.com>
<ME3P282MB3166E42AEB15A41E6EBADC67B62C2@ME3P282MB3166.AUSP282.PROD.OUTLOOK.COM>
<CAPpHfduSX=7sEP7yp_ecsjaBDogXfHgXZ75Mg802r9XoJLxJLQ@mail.gmail.com>
<CALT9ZEHLPS8ZJVH4MyszsH_-quxPgH_rh7ZuaVYiedHx_tnzqA@mail.gmail.com>
<CALT9ZEHNeagO5PLb4Nv9J_ZaCtp+ArdVmbSLc0RHUzx_RPAa4w@mail.gmail.com>
<CALT9ZEFVRmQfFrawTnHNCRxY1Nc+gV-wbZKrCKO4H9kA0i-8tg@mail.gmail.com>
<CAPpHfdv6fDMkVZyJKuoaYdg5+zVnRNU6dMRM9m4mBH6+4W89ig@mail.gmail.com>
Hi, Alexander!
The revised rest of the patchset is attached.
> 0001 (was 0006) – I prefer the definition of AcquireSampleRowsFunc to
> stay in vacuum.h. If we move it to sampling.h then we would have to
> add there includes to define Relation, HeapTuple etc. I'd like to
> avoid this kind of change. Also, I've deleted
> table_beginscan_analyze(), because it's only called from
> tableam-specific AcquireSampleRowsFunc. Also I put some comments to
> heapam_scan_analyze_next_block() and heapam_scan_analyze_next_tuple()
> given that there are now no relevant comments for them in tableam.h.
> I've removed some redundancies from acquire_sample_rows(). And added
> comments to AcquireSampleRowsFunc based on what we have in FDW docs
> for this function. Did some small edits as well. As you suggested,
> turned back declarations for acquire_sample_rows() and compare_rows().
>
In my comment in the thread I was not thinking about returning the old name
acquire_sample_rows(), it was only about the declarations and the order of
functions to be one code block. To me heapam_acquire_sample_rows() looks
better for a name of heap implementation of *AcquireSampleRowsFunc(). I
suggest returning the name heapam_acquire_sample_rows() from v4. Sorry for
the confusion in this.
The changed type of static function that always returned true for heap
looks good to me:
static void heapam_scan_analyze_next_block
The same is for removing the comparison of always true "block_accepted" in
(heapam_)acquire_sample_rows()
Removing table_beginscan_analyze and call scan_begin() is not in the same
style as other table_beginscan_* functions. Though this is not a change in
functionality, I'd leave this part as it was in v4. Also, a comment about
it was introduced in v5:
src/backend/access/heap/heapam_handler.c: * with table_beginscan_analyze()
For comments I'd propose:
%s/In addition, store estimates/In addition, a function should store
estimates/g
%s/zerp/zero/g
> 0002 (was 0007) – I've turned the redundant "if", which you've pointed
> out, into an assert. Also, added some comments, most notably comment
> for TableAmRoutine.reloptions based on the indexam docs.
>
%s/validate sthe/validates the/g
This seems not needed, it's already inited to InvalidOid before.
+else
+accessMethod = default_table_access_method;
+ accessMethodId = InvalidOid;
This code came from 374c7a22904. I don't insist on this simplification in a
patch 0002.
Overall both patches look good to me.
Regards,
Pavel Borisov.
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Table AM Interface Enhancements
In-Reply-To: <CALT9ZEFb0yVmZ_OQACFhYDEP3OSo4BPAMk9kju2Ap274D6SqJg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox