On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote: > > Hi, > > On Wed, Oct 09, 2024 at 10:21:31AM -0700, Masahiko Sawada wrote: > > On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot > > <bertranddrouvot...@gmail.com> wrote: > > > One option could be (did not test it) to add this switch in > > > construct_array_builtin(): > > > > > > + case XIDOID: > > > + elmlen = sizeof(TransactionId); > > > + elmbyval = true; > > > + elmalign = TYPALIGN_INT; > > > + break; > > > > > > I think that could make sense and would probably need a dedicated patch > > > for that, > > > thoughts? > > > > Or can we use construct_array() instead? > > I had a closer look to d746021de1 (which introduced construct_array_builtin()) > and the hackers thread that lead to it [1]. > > IIUC, the idea was to: > > 1. centralize the hardcoded knowledge that were in the calls to > construct_array() > and deconstruct_array() for built-in types > 2. notational simplification and bug-proofing > > As XIDOID is a built-in type, I think that it would make sense to add it in > deconstruct_array_builtin()/construct_array_builtin(). > > I think the reason XIDOID has not been added in d746021de1 is that there were > no > use case at that time (means no existing calls to > construct_array()/deconstruct_array() > with hardcoded XIDOID related arguments). > > One could say that we would just add 2 calls to construct_array() in the > pg_logicalinspect > module but, for example, d746021de1 also took care of CSTRINGOID that had a > single > call at that time: > > $ git show d746021de1 | grep deconstruct_array_builtin | grep -c CSTRINGOID > 1 > $ git show d746021de1 | grep construct_array_builtin | grep -v > deconstruct_array_builtin | grep -c CSTRINGOID > 1 > > So I think that having construct_array_builtin()/deconstruct_array_builtin() > taking care of XIDOID is the way to go. If that makes sense to you then I'll > submit a dedicated patch for it, thoughts?
Your explanation makes sense to me. I think it can be included in the main pg_logicalinspect patch as this change is a part of it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
