Re: Table AM Interface Enhancements - Mailing list pgsql-hackers
From | Alexander Korotkov |
---|---|
Subject | Re: Table AM Interface Enhancements |
Date | |
Msg-id | CAPpHfduUH1iYPtTBMzeyejy38fR8cWohPhje6n-DoD0fk1=v7A@mail.gmail.com Whole thread Raw |
In response to | Re: Table AM Interface Enhancements (Andres Freund <andres@anarazel.de>) |
Responses |
Re: Table AM Interface Enhancements
Re: Table AM Interface Enhancements |
List | pgsql-hackers |
On Mon, Apr 15, 2024 at 11:11 PM Andres Freund <andres@anarazel.de> wrote: > On 2024-04-12 01:04:03 +0300, Alexander Korotkov wrote: > > 1) If we just apply my revert patch and leave c6fc50cb4028 and > > 041b96802ef in the tree, then we get our table AM API narrowed. As > > you expressed the current API requires block numbers to be 1:1 with > > the actual physical on-disk location [2]. Not a secret I think the > > current API is quite restrictive. And we're getting the ANALYZE > > interface narrower than it was since 737a292b5de. Frankly speaking, I > > don't think this is acceptable. > > As others already pointed out, c6fc50cb4028 was committed quite a while > ago. I'm fairly unhappy about c6fc50cb4028, fwiw, but didn't realize that > until it was too late. +1 > > In token of all of the above, is the in-tree state that bad? (if we > > abstract the way 27bc1772fc and dd1f6b0c17 were committed). > > To me the 27bc1772fc doesn't make much sense on its own. You added calls > directly to heapam internals to a file in src/backend/commands/, that just > doesn't make sense. > > Leaving that aside, I think the interface isn't good on its own: > table_relation_analyze() doesn't actually do anything, it just sets callbacks, > that then later are called from analyze.c, which doesn't at all fit to the > name of the callback/function. I realize that this is kinda cribbed from the > FDW code, but I don't think that is a particularly good excuse. > > I don't think dd1f6b0c17 improves the situation, at all. It sets global > variables to redirect how an individual acquire_sample_rows invocation > works: > void > block_level_table_analyze(Relation relation, > AcquireSampleRowsFunc *func, > BlockNumber *totalpages, > BufferAccessStrategy bstrategy, > ScanAnalyzeNextBlockFunc scan_analyze_next_block_cb, > ScanAnalyzeNextTupleFunc scan_analyze_next_tuple_cb) > { > *func = acquire_sample_rows; > *totalpages = RelationGetNumberOfBlocks(relation); > vac_strategy = bstrategy; > scan_analyze_next_block = scan_analyze_next_block_cb; > scan_analyze_next_tuple = scan_analyze_next_tuple_cb; > } > > Notably it does so within the ->relation_analyze tableam callback, which does > *NOT* not actually do anything other than returning a callback. So if > ->relation_analyze() for another relation is called, the acquire_sample_rows() > for the earlier relation will do something different. Note that this isn't a > theoretical risk, acquire_inherited_sample_rows() actually collects the > acquirefunc for all the inherited relations before calling acquirefunc. You're right. No sense trying to fix this. Reverted. ------ Regards, Alexander Korotkov
pgsql-hackers by date: