MIME-Version: 1.0
References: <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
 <Z5cDQJM9nnoXuStO@paquier.xyz> <Z5xTb5iBHVGns35R@paquier.xyz>
 <CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@mail.gmail.com>
 <Z5xgmgyV-wnzMpkO@paquier.xyz> <CAA5RZ0vM9AsEqvKued2drKZJ1opt3wbYaDbxGzi-khkNzwn7og@mail.gmail.com>
 <Z6Ks3ta5089DXiPc@paquier.xyz> <CAA5RZ0sfRbd1xcq_oHNA0TPr57yM8qkg-GoD6A0nUfyxZhs33Q@mail.gmail.com>
 <CAP53PkxocbNr+eRag3FEJp3-7S1U80FspOg8UQjO902TWMG=6A@mail.gmail.com>
 <CAA5RZ0u6yJdFL=p5vdpbZFS-2YY+Z6vtzmt4gejgZa3RcNiWMQ@mail.gmail.com> <Z6lVcWdNQtgz9BGI@paquier.xyz>
In-Reply-To: <Z6lVcWdNQtgz9BGI@paquier.xyz>
From: Sami Imseih <samimseih@gmail.com>
Date: Mon, 10 Feb 2025 14:02:10 -0600
Message-ID: <CAA5RZ0t80hP2aTv97QtEJy39GkxKmDBVDiTBApfiuTa4O=TEWQ@mail.gmail.com>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
To: Michael Paquier <michael@paquier.xyz>
Cc: Lukas Fittl <lukas@fittl.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Marko M <marko@pganalyze.com>
Content-Type: multipart/mixed; boundary="000000000000b4aa02062dcf2fdd"
Archived-At: <https://www.postgresql.org/message-id/CAA5RZ0t80hP2aTv97QtEJy39GkxKmDBVDiTBApfiuTa4O%3DTEWQ%40mail.gmail.com>
Precedence: bulk

--000000000000b4aa02062dcf2fdd
Content-Type: text/plain; charset="UTF-8"

> 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

--000000000000b4aa02062dcf2fdd
Content-Type: application/octet-stream; 
	name="v1-0001-Fix-comment-indentation.patch"
Content-Disposition: attachment; 
	filename="v1-0001-Fix-comment-indentation.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_m6zh6z540>
X-Attachment-Id: f_m6zh6z540

RnJvbSBhMjU2MDA2NjkxYWJlZDNlYjNlNDAxZTVmYjlmYjMwZTA4ZDFmMTlhIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiAiU2FtaSBJbXNlaWggKEFXUykiCiA8c2ltc2VpaEBkZXYtZHNr
LXNpbXNlaWgtMWQtMzk0MGI3OWUudXMtZWFzdC0xLmFtYXpvbi5jb20+CkRhdGU6IE1vbiwgMTAg
RmViIDIwMjUgMTg6MDE6MzEgKzAwMDAKU3ViamVjdDogW1BBVENIIHYxIDEvMl0gRml4IGNvbW1l
bnQgaW5kZW50YXRpb24KCkZpeCBhbiB1bmluZGVudGVkIGNvbW1lbnQgaW50cm9kdWNlZCBpbgoz
ZDE3ZDdkN2ZiNwotLS0KIHNyYy9pbmNsdWRlL25vZGVzL3BsYW5ub2Rlcy5oIHwgMiArLQogMSBm
aWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEv
c3JjL2luY2x1ZGUvbm9kZXMvcGxhbm5vZGVzLmggYi9zcmMvaW5jbHVkZS9ub2Rlcy9wbGFubm9k
ZXMuaAppbmRleCA2N2U0MDQwYTcwLi5iZjFmMjVjMGRiIDEwMDY0NAotLS0gYS9zcmMvaW5jbHVk
ZS9ub2Rlcy9wbGFubm9kZXMuaAorKysgYi9zcmMvaW5jbHVkZS9ub2Rlcy9wbGFubm9kZXMuaApA
QCAtMTEwLDcgKzExMCw3IEBAIHR5cGVkZWYgc3RydWN0IFBsYW5uZWRTdG10CiAJICovCiAJTGlz
dAkgICAqc3VicGxhbnM7CiAKLS8qIGluZGljZXMgb2Ygc3VicGxhbnMgdGhhdCByZXF1aXJlIFJF
V0lORCAqLworCS8qIGluZGljZXMgb2Ygc3VicGxhbnMgdGhhdCByZXF1aXJlIFJFV0lORCAqLwog
CUJpdG1hcHNldCAgKnJld2luZFBsYW5JRHM7CiAKIAkvKiBhIGxpc3Qgb2YgUGxhblJvd01hcmsn
cyAqLwotLSAKMi40Ny4xCgo=
--000000000000b4aa02062dcf2fdd--