Query Errors if pg_hint_plan.enable_hint_table and enable_hint is enabled but hints table does not exist #191
Comments
samimseih
changed the title
|
I took another look. If the user creates the At minimum, should this scenario be highlighted in docs [1]? Thoughts? |
|
One thing that we could do is a check callback for the GUC to ease the error message when using a SET. Like for check_default_table_access_method, it cannot be perfect: it is not possible to do a catalog lookup to see if the hint table exists or if the extension is installed if not connected to a database and if not within a transaction. |
To be sure, you are thinking to check if the pg_hint_plan extension has been created in the guc check hook? |
Or check that the table exists. Both require a catalog lookup and a transaction state, so that could run under the same constraints. LImited because of these constraints, still perhaps useful for some. |
|
such a check would indeed be very limited, and totally useless in some scenarios that I would expect to be common (like using session_preload_libraries). it also wouldn't cover e.g. persistent connections and a later drop extension. it would probably be better to add a check in get_hints_from_table() that the table exist (like trying to get its oid or something) and raise a clear error message with a hint. |
|
Hmm. Is an error message the best choice though? If pg_hint_plan.enable_hint_table is enabled in postgresql.conf, then one would get an error for basically all queries run in the backend. I have been annoyed by that a bit myself. So how about lowering this log's level? WARNING would noisy and bloat the logs, though. |
|
On the other hand, if you rely on the extension being installed to not have your production server crash and burn due to incorrect plans, downgrading the message to WARNING would decrease the chances to notice the problem immediately after a maintenance operation. Ideally we should document that enabling it globally is likely to be a terrible idea and should be done at the database level using ALTER DATABASE SET. |
That's a good point. It's better to error out rather than let query execution go sideways leaving the user confused as to why. Adding more documentation are this behavior is necessary at minimum. |
Yeah, agreed that this looks like a good alternative based on the fact that CREATE EXTENSION needs to happen for each database. |
|
I've also wondered if the hints table can be turned into a dynamic-sized hash table; which may be a better way to go long term particularly if we remove the normalized_text and start using queryId. We will have to deal with persisting the data across reboots and crashes. |
|
We could do that, but there will still be the fundamental requirement that query identifiers are only guaranteed to be meaningful on a per-database level (unless you have a jumbling extension that only works with object names rather than oid, but we can't assume that by default), so you will need to add the database oid in the hash key and declare them on a per-database basis anyway. Given those requirements I'm not sure that the extra infrastructure to handle it will be really worth it. |
|
It may still be worth considering. There could probably be performance benefits with going down the hash table path, such as the data always being in memory ( a heavily used hints table will probably always be in memory, so that's probably not a major gain), but there is also the additional locking required for a normal table and the extra sql to fetch the hint. |
I guess that you are also referring here to the part where the data would need to persist across restarts and fill in the hash table when starting without crash recovery, right? |
Exactly. And unless we want to do the configuration using some SQL wrappers on top of C functions and declare them manually on each replica, the source of truth will probably stay in some local table, so this would only be a cache so it will also need to handle eviction and refreshing based on the source table, handle the MVCC aspect and so on. Having such a cache will likely improve performance, as @samimseih pointed, but I don't think it's really trivial to do. |
I'm biased on the risk vs benefit risk on this one and there are a few other things I'd like to get done around first, so I don't really plan to work on that. |
@samimseih Would you like to take a stab on these two points? If we are to do something, I'm OK with a clear error message if the GUC is enabled but the extension has not been created. That would be more useful than the current error message, and one would know what to do (aka mention in the error message to run CREATE EXTENSION). |
|
@michaelpq Yes, I will look into providing a patch for this. |
We can do a catalog check inside What do you think? |
|
Hmm.. We could get in get_hints_from_table() only if pg_hint_plan_enable_hint_table is enabled, and it is disabled by default. I'm OK to be consistent with compute_query_id being disabled on the fly, and just force a WARNING. It is true that incorrect hints do not force a query error; we just skip the hint. |
|
The attached patch 1/ adds handling in 2/ Improved documentation asking the user to set 0001-v1-Handle-missing-hint_plan.hints-table-when-pg_hint_pl.patch |
|
Could it be simpler to just do a get_extension_oid() and see if the extension is installed? The table depends on the extension to exist. |
From what I can tell, pg_extension does not have a syscache. It will be slightly simpler to check for extension, but less efficient and we care about performance in What do you think? |
Yeah.. There was a patch in upstream to add a syscache to extensions, which I'm going to apply for v18~, first. I could live with the namespace and table lookup for ~17. |
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
An environment enabling pg_hint_plan.enable_hint_table while pg_hint_plan has not been created as an extension would fail all its queries as the table created by the extension does not exist. This commit adds a check at the top of get_hints_from_table() to check if the extension is installed before attempting to use the hint table, generating a WARNING. We have discussed about a few options, but a WARNING is more consistent with how duplicated hints or compute_query_id disabled are handled. This does not completely close the failure hole, though, as it is still possible to see the table lookup failure for the CREATE EXTENSION command enabling pg_hint_plan as an extension if enable_hint_table is enabled. In terms of code simplicity, I am not really convinced that we need to do more just for that. This commit relies on 490f869d92e5 that has introduced syscache entries for pg_extension. On stable branches, we will need a different logic with a check on the table itself with its namespace. Idea based on some feedback from Julien Rouhaud. The tests are from me. Author: Sami Imseih, Michael Paquier Backpatch-through: 12 Per issue #191.
|
And done on all stable branches, with some tests. |
if a user enables
enable_hint_tableandenable_hintbut thehint_plan.hintsdoes not exist, it will lead to query failure.I don't think this should result in an error, but should be silently ignored ( with a message with
debug_print)Regards,
Sami
The text was updated successfully, but these errors were encountered: