public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
Date: Thu, 10 Oct 2024 00:05:10 -0700
Message-ID: <CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@mail.gmail.com> (raw)
In-Reply-To: <ZwdK1UYdLn/[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAD21AoBHkL-C22pgGJCA_8Jv6HjYq0c_1Y_b2Xktk7_SdUqxqA@mail.gmail.com>
<ZwOkPDCKQ/[email protected]>
<CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
<[email protected]>
<CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
<ZwY68OhKx/[email protected]>
<CAD21AoCAEXmxaDHAjMwqcpV6JVa91wBbXamQbk-rv1QnYhzGhA@mail.gmail.com>
<ZwdK1UYdLn/[email protected]>
On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot
<[email protected]> 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
> > <[email protected]> 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
view thread (36+ 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], [email protected], [email protected], [email protected]
Subject: Re: Add contrib/pg_logicalsnapinspect
In-Reply-To: <CAD21AoB-bxBqBEt9zbUQKTTUag1_NRPeT_5aGXC_j+z0+T3Hkw@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