On Fri, Jul 21, 2023 at 10:42 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Gurjeet Singh <gurj...@singh.im> writes: > > Please see attached the patch that does this. Let me know if this patch > > helps. > > I don't like this patch one bit, because it adds a lot of overhead > (i.e., an extra index_open/close cycle for every btree index in every > query) to support a tiny minority use-case.
I anticipated the patch's performance impact may be a concern, but before addressing it I wanted to see if the patch actually helped Index Adviser. Ahmed has confirmed that my proposed patch works for him. I believe the additional index_open() would not affect the performance significantly, since the very same indexes were index_open()ed just before calling the get_relation_info_hook. All the relevant caches would be quite fresh because of the index_open() in the same function above. And since the locks taken on these indexes haven't been released, we don't have to work hard to take any new locks (hence the index_open() with NoLock flag). > How come we don't > already know whether the index is hypothetical at the point where > _bt_getrootheight is called now? Because the 'hypthetical' flag is not stored in catalogs, and that's okay; see below. At that point, the only indication that an index may be a hypothetical index is if RelationGetNumberOfBlocks() returns 0 for it, and that's what Ahmed's proposed patch relied on. But I think extrapolating that info->pages==0 implies it's a hypothetical index, is stretching that assumption too far. > Actually, looking at the existing comment at the call site: > > /* > * Allow a plugin to editorialize on the info we obtained from the > * catalogs. Actions might include altering the assumed relation size, > * removing an index, or adding a hypothetical index to the indexlist. > */ > if (get_relation_info_hook) > (*get_relation_info_hook) (root, relationObjectId, inhparent, rel); > > reminds me that the design intention was that hypothetical indexes > would get added to the list by get_relation_info_hook itself. > If that's not how the index adviser is operating, maybe we need > to have a discussion about that. Historically, to avoid having to hand-create the IndexOptInfo and risk getting something wrong, the Index Adviser has used index_create() to create a full-blown btree index, (sans that actual build step, with skip_build = true), and saving the returned OID. This ensured that all the catalog entries were in place before it called the standard_planner(). This way Postgres would build IndexOptInfo from the entries in the catalog, as usual. Then, inside the get_relation_info_hook() callback, Index Adviser identifies these virtual indexes by their OID, and at that point marks them with hypothetical=true. After planning is complete, the Index Adviser scans the plan to find any IndexScan objects that have indexid matching the saved OIDs. Index Adviser performs the whole thing in a subtransaction, which gets rolled back. So the hypothetical indexes are not visible to any other transaction, ever. Assigning OID to a hypothetical index is necessary, and I believe index_create() is the right way to do it. In fact, in the 9.1 cycle there was a bug fixed (a2095f7fb5, where the hypothetical flag was also invented), to solve precisely this problem; to allow the Index Adviser to use OIDs to identify hypothetical indexes that were used/chosen by the planner. But now I believe this architecture of the Index Adviser needs to change, primarily to alleviate the performance impact of creating catalog entries, subtransaction overhead, and the catalog bloat caused by index_create() (and then rolling back the subtransaction). As part of this architecture change, the Index Adviser will have to cook up IndexOptInfo objects and append them to the relation. And that should line up with the design intention you mention. But the immediate blocker is how to assign OIDs to the hypothetical indexes so that all hypothetical indexes chosen by the planner can be identified by the Index Adviser. I'd like the Index Adviser to work on read-only /standby nodes as well, but that wouldn't be possible because calling GetNewObjectId() is not allowed during recovery. I see that HypoPG uses a neat little hack [1]. Perhaps Index Adviser will also have to resort to that trick. [1]: hypo_get_min_fake_oid() finds the usable oid range below FirstNormalObjectId https://github.com/HypoPG/hypopg/blob/57d832ce7a2937fe7d42b113c7e95dd1f129795b/hypopg.c#L458 Best regards, Gurjeet http://Gurje.et