> On Aug 26, 2024, at 5:21 AM, Peter Eisentraut <pe...@eisentraut.org> wrote:
>
> On 21.08.24 21:25, Mark Dilger wrote:
>> The next twenty patches are a mix of fixes of various layering
>> violations, such as not allowing non-core index AMs from use in replica
>> identity full, or for speculative insertion, or for foreign key
>> constraints, or as part of merge join; with updates to the "treeb" code
>> as needed. The changes to "treeb" are broken out so that they can also
>> easily be excluded from whatever gets committed.
>
> I made a first pass through this patch set.
Peter, thanks for the review!
> I think the issues it aims to address are mostly legitimate. In a few cases,
> we might need some more discussion and perhaps will end up slicing the APIs a
> bit differently. The various patches that generalize the strategy numbers
> appear to overlap with things being discussed at [0], so we should see that
> the solution covers all the use cases.
>
> [0]:
> https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mdhcy4_qq0+noc...@mail.gmail.com
Paul, it seems what you are doing in
v39-0001-Add-stratnum-GiST-support-function.patch is similar to what I am doing
in v17-0012-Convert-strategies-to-and-from-row-compare-types.patch. In
particular, your function
+
+/*
+ * Returns the btree number for supported operators, otherwise invalid.
+ */
+Datum
+gist_stratnum_btree(PG_FUNCTION_ARGS)
+{
+ StrategyNumber strat = PG_GETARG_UINT16(0);
+
+ switch (strat)
+ {
+ case RTEqualStrategyNumber:
+ PG_RETURN_UINT16(BTEqualStrategyNumber);
+ case RTLessStrategyNumber:
+ PG_RETURN_UINT16(BTLessStrategyNumber);
+ case RTLessEqualStrategyNumber:
+ PG_RETURN_UINT16(BTLessEqualStrategyNumber);
+ case RTGreaterStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterStrategyNumber);
+ case RTGreaterEqualStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterEqualStrategyNumber);
+ default:
+ PG_RETURN_UINT16(InvalidStrategy);
+ }
+}
looks similar to the implementation of an amtranslate_rctype_function. Do you
have any interest in taking a look?
> To make a dent, I picked out something that should be mostly harmless: Stop
> calling directly into _bt_getrootheight() (patch 0004). I think this patch
> is ok, but I might call the API function amgettreeheight instead of
> amgetrootheight. The former seems more general.
Peter, your proposed rename seems fine for the current implementation, but your
suggestion below might indicate a different naming.
> I notice that _bt_getrootheight() is called only to fill in the IndexOptInfo
> tree_height field, which is only used by btcostestimate(), so in some sense
> this is btree-internal data. But making it so that btcostestimate() calls
> _bt_getrootheight() directly to avoid all that intermediate business seems
> too complicated, and there was probably a reason that the cost estimation
> functions don't open the index.
>
> Interestingly, the cost estimation functions for gist and spgist also look at
> the tree_height field but nothing ever fills it on. So with your API
> restructuring, someone could provide the missing API functions for those
> index types. Might be interesting.
>
> That said, there might be value in generalizing this a bit. If you look at
> the cost estimation functions in pgvector (hnswcostestimate() and
> ivfflatcostestimate()), they both have this pattern that btcostestimate()
> tries to avoid: They open the index, look up some number, close the index,
> then make a cost estimate computation with the number looked up. So another
> idea would be to generalize the tree_height field to some "index size data"
> or even "internal data for cost estimation". This wouldn't need to change
> the API much, since these are all just integer values, but we'd label the
> functions and fields a bit differently.
Would they be just integers? They could also be void*, with
amgetrootheight_function returning data allocated in the current memory
context. For btree, that would just be a four byte integer, but other indexes
could return whatever they like. If you like that idea, I can code that up for
v18, naming the field something like amgetcostestimateinfo_function.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
