public inbox for [email protected]
help / color / mirror / Atom feedFrom: Sami Imseih <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Lukas Fittl <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Marko M <[email protected]>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
Date: Mon, 10 Feb 2025 14:02:10 -0600
Message-ID: <CAA5RZ0t80hP2aTv97QtEJy39GkxKmDBVDiTBApfiuTa4O=TEWQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@mail.gmail.com>
<[email protected]>
<CAA5RZ0vM9AsEqvKued2drKZJ1opt3wbYaDbxGzi-khkNzwn7og@mail.gmail.com>
<[email protected]>
<CAA5RZ0sfRbd1xcq_oHNA0TPr57yM8qkg-GoD6A0nUfyxZhs33Q@mail.gmail.com>
<CAP53PkxocbNr+eRag3FEJp3-7S1U80FspOg8UQjO902TWMG=6A@mail.gmail.com>
<CAA5RZ0u6yJdFL=p5vdpbZFS-2YY+Z6vtzmt4gejgZa3RcNiWMQ@mail.gmail.com>
<[email protected]>
> This fixes the long comments in plannodes.h to make it easier to add the
> attribute annotation. It made the most sense to make this the first patch
> in the set.
> A commit that happened last Friday made also this to have conflict.
Thanks for committing v5-0001. I am not sure why there is comment
that is not correctly indented. Attached is a fix for that
> I don't understand why there is a need for publishing AppendJumble()
> while it remains statis in jumblefuncs.c. This is not needed in 0003
> and 0004, either.
In v5-0002, AppendJumble is no longer static.
-static void
+void
AppendJumble(JumbleState *jstate, const unsigned char *item, Size size)
{
Maybe I am missing something?
> Should we use more generic names for the existing custom_query_jumble,
> no_query_jumble, query_jumble_ignore and query_jumble_location? Last
> time I've talked about that with Peter E, "jumble" felt too generic,
> so perhaps we're looking for a completely new term? This impacts as
> well the naming of the existing queryjumblefuncs.c. The simplest term
> that may be possible here is "hash", actually, because that's what we
> Point is that query_jumble_ignore is used in the planner nodes, which
> feels inconsistent, so perhaps we could rename query_jumble_ignore and
> no_query_jumble to "hash_ignore" and/or "no_hash", or something like
> that? This may point towards the need of a split, not sure, still the
> picture is incomplete.
I was thinking about this as I was reworking the comments in jumblefuncs.c
for v5-0002.
I am OK with moving away from "jumble" in-lieu of something else, but
my thoughts are we should actually call this process "fingerprint"
( a term we already use in the queryjumblefuncs.c comment ).
A fingerprint consists of all the interesting parts of a node tree that are
appended and the final product is a hash of this fingerprint ( i.e. queryId )
For node attributes we can specify "fingerprint_ignore"
or "no_fingerprint". What do you think?
> The concept of location does not apply to plans, based on the
> current proposal, so perhaps we should talk about "query normalization
> location"?
Are you referring to JUMBLE_LOCATION? and whether to keep it in
queryjumblefuncs.c ( or jumblefuncs.c as is being proposed )?
Regards,
Sami
Attachments:
[application/octet-stream] v1-0001-Fix-comment-indentation.patch (815B, 2-v1-0001-Fix-comment-indentation.patch)
download | inline diff:
From a256006691abed3eb3e401e5fb9fb30e08d1f19a Mon Sep 17 00:00:00 2001
From: "Sami Imseih (AWS)"
<[email protected]>
Date: Mon, 10 Feb 2025 18:01:31 +0000
Subject: [PATCH v1 1/2] Fix comment indentation
Fix an unindented comment introduced in
3d17d7d7fb7
---
src/include/nodes/plannodes.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 67e4040a70..bf1f25c0db 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -110,7 +110,7 @@ typedef struct PlannedStmt
*/
List *subplans;
-/* indices of subplans that require REWIND */
+ /* indices of subplans that require REWIND */
Bitmapset *rewindPlanIDs;
/* a list of PlanRowMark's */
--
2.47.1
view thread (34+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
In-Reply-To: <CAA5RZ0t80hP2aTv97QtEJy39GkxKmDBVDiTBApfiuTa4O=TEWQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox