On Thu, 22 May 2025 at 02:58, Peter Eisentraut <pe...@eisentraut.org> wrote: > > On 20.05.25 08:38, Michael Paquier wrote: > > On Tue, May 20, 2025 at 05:51:51PM +1200, David Rowley wrote: > >> Given the planId stuff is new and has the same issue, I think that > >> pushes me towards thinking now is better than later for fixing both. > >> > >> I'm happy to adjust my patch unless you've started working on it already. > > > > Here you go with the attached, to-be-applied on top of your own patch. > > Whichever way we're going, surely this whole thing could benefit from a > > typedef something QueryId;
This part I'm not so sure about. It's not as if it can be changed to some other type with a simple definition of the typedef. Looking at the patch I proposed there are quite a few things that would still need to be updated after the typedef is changed. PG_GETARG_INT64(), INT64CONST() and DatumGetInt64() are part of that. There's also the return type of hash_any_extended(). Changing the typedef definition might also need an adjustment in gen_node_support.pl, depending on what you're changing it to. You could argue that if it reduces the locations that need to be changed by using a typedef, then it's a win. But there are also negative aspects to typedefs that need to be considered. For me, those are the added level of indirection of code reading to actually who what type I'm working with. I personally dislike typedefs like "typedef PageHeaderData *PageHeader;" as they hide the fact I'm working with a pointer. I'm not outright objecting to the typedef for this. It's just I don't see it as a clear-cut improvement for this case. David
