Open danolivo opened 1 year ago
I totally agree that the hash approach has a lot of drawbacks and isn't a long term solution.
I'm just using it for now as a simple way to develop and test this extension, as I can e.g. run the postgres regression tests with this extension enabled and check that everything works just fine and I don't leak memory everywhere without writing a specific test for all possible cases (and while hash collisions are possible with the double hash approach they're just so unlikely that it works just fine for that scenario).
The long term plan I had was to hijack into standard prepared statements infrastructure and implement some kind of "global prepared statement" with a dedicated GUC in that extension. In that mode a PREPARE can be saved in shared memory rather than local memory and EXECUTE will retrieve it from there too. In that case, the key will be the prepared statement name rather than some hash, so all the problem you mentioned should disappear.
One more question is: it is better to store parameterised plan instead of the constant one.
I'm not entirely sure of what you mean by that. Are you talking about manually removing the constant that might still be present in a plan and "parameterize it" manually to avoid storing almost-identical plans?
In that mode a PREPARE can be saved in shared memory rather than local memory
In the case of the core patch, why not to store such a plan in a catalog relation (_pg_frozenplans, for example) and pull it up on-demand by a backend separately, into local PlanCache?
I'm not entirely sure of what you mean by that. Are you talking about manually removing the constant that might still be present in a plan and "parameterize it" manually to avoid storing almost-identical plans?
I have made it in more trivial way:
In toto, we should use specific implementation of QueryJumble(), have some overhead on replacement of specific Const nodes in parse tree and overhead on trees comparison. But we have a good chance (depends on queryId uniqueness) to quickly find stored statement and avoid planning/optimization procedure at all.
In the case of the core patch, why not to store such a plan in a catalog relation (pg_frozen_plans, for example) and pull it up on-demand by a backend separately, into local PlanCache?
What do you mean by "core patch"? Apart from that, one of the goal of a global cache is to avoid duplicating the same data in all backends and save memory so I hope that the local plancache can entirely be bypassed.
I have made it in more trivial way:
I'm still unsure of what exactly you want to achieve here. If that's what I mentioned before:
manually removing the constant that might still be present in a plan and "parameterize it" manually to avoid storing almost-identical plans
I'm not sure how beneficial it can be. Having a known constant at plan time usually leads to way better plans and most applications are probably relying on that when they hardcode some values rather than using parameters, so naively I'm a bit skeptical that adding overhead to prevent that is really sensible.
avoid duplicating the same data in all backends and save memory
I've got it now, thanks. You just try to make 'shared plancache' for prepared statements to make them global, no more.
I'm a bit skeptical that adding overhead to prevent that is really sensible
Got it too. I'm working on very different use case - to guarantee a user what the plan will not changed for a query across all backends for specific statement - kind of manual plan freezing - obviously, not a PostgreSQL core feature, pure enterprise.
If to talk in the context of global prepared statements without local cache, I see two main problems: transactional visibility and a plan invalidation procedure - the case of aborted transaction, which have executed some DDL.
Also, thanks for your time and explanation!
Ah, now that I know your usecase it all makes more sense! Yes indeed our goals are radically different and actually quite opposed as I indeed have to make sure that plans will change when needed.
You're correct, transactions and invalidations are things that needs to be handled, and added some code to handle such cases which seems to work fine. It could still be improved though, as for now to handle the "transaction that executes some DDL and rollback" case, the entry is evicted inside the transaction and no new plan can be cached until the transaction is committed. It's probably ok for most cases, but if you have a long running transaction it can probably be problematic in some cases.
I guess, most of conceptual problems will be the same. So, do you think you could keep me posted if you will debate this feature somewhere?
Do you mean if I plan to work on a "plan freeze" feature?
Not only. Global prepared statement, plan invalidation issue and so on. One more debatable question which also interested to discuss is a plan portability - could we transfer it to standby and have chances for even more cool feature - design some import/export procedure for a plan transfer to another instance with some safety guarantees.
Not only. Global prepared statement, plan invalidation issue and so on.
Ah I see. Well global prepared statement is definitely the next thing on the list. Plan invalidation, transactions with DDL, rollbacked or not are already handled, at least partially (see the comment on top of pgsp_ProcessUtility).
One more debatable question which also interested to discuss is a plan portability - could we transfer it to standby and have chances for even more cool feature - design some import/export procedure for a plan transfer to another instance with some safety guarantees.
It would be quite hard to do. You don't have the RelationCacheInvalidate mechanism on a standby, nor can you hook into standard_processUtility and other, and you need those to handle invalidations. So the only way to make it work on a standby would be to WAL log everything happening on hash table, and you can't really do that unless you make all the data fully durable, assuming that the current AccessExclusiveLock replication is enough to guarantee proper isolation.
I think, hash-based approach have some caveats (rewritten queries or collisions as an example). To correctly prove, that query could be executed with the plan, we should store also a parse tree and compare the parse tree of incoming query with the stored one. Of course, it is not a cheap procedure. And here queryId can help - just to reduce space of search up to one or two shared plans. One more question is: it is better to store parameterised plan instead of the constant one. For simplicity, parameterised parts of a query may be defined manually, by an user. So, we just need to re-design equal() func a bit, to allow a specific mode. In such a specific comparing mode equal() can check: has shared plan a parameter of specific type in the position of the Const or not? If yes - we can use such a generic plan for execution of the incoming statement ...