> AFAICS you're proposing to add an identifier for a specific plan, but no
> way to
> know what that plan was? How are users supposed to use the information if
> they
> know something changed but don't know what changed exactly?
I see this as a start to do more useful things with plans.
The patch right out the gate exposes the plan_id in EXPLAIN output
and auto_explain.
This will also be useful for extensions that will provide the plan text.
It is also conceivable that pg_stat_statements can provide an option
To store the plan text?
> - In the POC, the compute_query_id GUC determines if a
> plan_id is to be computed. Should this be a separate GUC?
> Probably, as computing it will likely be quite expensive. Some benchmark
> on
> various workloads would be needed here.
Yes, more benchmarks will be needed here with more complex plans. I have
Only benchmarked with pgbench at this point.
However, separating this into Its own GUC is what I am leaning towards as well
and will update the patch.
> I only had a quick look at the patch, but I see that you have some code to
> avoid storing the query text multiple times with different planid. How
> does it
> work exactly, and does it ensure that the query text is only removed once
> the
> last entry that uses it is removed? It seems that you identify a specific
> query text by queryid, but that seems wrong as collision can (easily?)
> happen
> in different databases. The real identifier of a query text should be
> (dbid,
> queryid).
The idea is to lookup the offsets and length of the text in the external file
by querid
only. Therefore we can store similar query text for multiple pgss_hash entries
only once.
When a new entry in pgss is not found, the new qtext_hash is consulted to
see if it has information about the offsets/length of the queryid. If found in
qtext_hash, the new pgss_hash entry is created with the offsets found.
If not found in qtext_hash, the query text will be (normalized) and stored in
the external file. Then, a new entry will be created in qtext_hash and
an entry in pgss_hash.
Of course we need to also handle the gc_qtext cleanups, entry_reset, startup
and shutdown scenarios. The patch does this, but I will go back and do more
testing.
> Note that this problem already exists, as the query texts are now stored
> per
> (userid, dbid, queryid, istoplevel). Maybe this part could be split in a
> different commit as it could already be useful without a planid.
Good point. I will separate this patch.
Regards,
Sami Imseih
Amazon Web Services
