Query Errors if pg_hint_plan.enable_hint_table and enable_hint is enabled but hints table does not exist

[samimseih]

if a user enables enable_hint_table and enable_hint but the hint_plan.hints does 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 )

postgres=# set pg_hint_plan.enable_hint_table = "on";
SET
postgres=# set pg_hint_plan.enable_hint = "on";
SET
postgres=# select 1;
ERROR:  relation "hint_plan.hints" does not exist
LINE 1: SELECT hints   FROM hint_plan.hints  WHERE norm_query_string...
                            ^
QUERY:  SELECT hints   FROM hint_plan.hints  WHERE norm_query_string = $1    AND ( application_name = $2     OR application_name = '' )  ORDER BY application_name DESC

Regards,

Sami

[samimseih]

I took another look. If the user creates the pg_hint_plan extension, this will not be an issue as the hints table is created as part of the extension. It also seems there will be difficulty in actually handling this error and proceeding with the query as the user must rollback the transaction.

At minimum, should this scenario be highlighted in docs [1]?

Thoughts?

[1] https://github.com/ossc-db/pg_hint_plan/blob/master/docs/hint_table.md#the-hint-table

[michaelpq]

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.

[samimseih]

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:

To be sure, you are thinking to check if the pg_hint_plan extension has been created in the guc check hook?

[michaelpq]

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.

[rjuju]

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.

[michaelpq]

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.

[rjuju]

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.

[samimseih]

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.

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.

[michaelpq]

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.

Yeah, agreed that this looks like a good alternative based on the fact that CREATE EXTENSION needs to happen for each database.

[samimseih]

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.

[rjuju]

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.

[samimseih]

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.

[michaelpq]

Given those requirements I'm not sure that the extra infrastructure to handle it will be really worth it.

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?

[rjuju]

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.

[michaelpq]

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.

[michaelpq]

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. 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.

@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).

[samimseih]

@michaelpq Yes, I will look into providing a patch for this.

[samimseih]

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.

We can do a catalog check inside get_hints_from_table to check if the hint_plans.hints exists. However, I am not convinced it's a good idea to raise an error. Wouldn't it be better to let the query proceed? and, If the user wants to know why a hint was not used, they can enable pg_hint_plan.debug_print.

What do you think?

[michaelpq]

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.

[samimseih]

The attached patch

1/ adds handling in get_hints_from_table() . When hint_plan.hints is missing, a warning is raised to the user asking them to run CREATE EXTENSION, and the function returns an empty hint string.

2/ Improved documentation asking the user to set pg_hint_plan.enable_hint_table by ALTER DATABASE SET.

0001-v1-Handle-missing-hint_plan.hints-table-when-pg_hint_pl.patch

[michaelpq]

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.

[samimseih]

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 get_hints_from_table.

What do you think?

[michaelpq]

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.

[michaelpq]

And done on all stable branches, with some tests.