Thread: Improve description of XLOG_RUNNING_XACTS
Hi, I realized that standby_desc_running_xacts() in standbydesc.c doesn't describe subtransaction XIDs. I've attached the patch to improve the description. Here is an example by pg_wlaldump: * HEAD rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 * w/ patch rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 subxacts: 1049 Please review it. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
On 2022/07/21 10:13, Masahiko Sawada wrote: > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > describe subtransaction XIDs. I've attached the patch to improve the > description. +1 The patch looks good to me. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
At Thu, 21 Jul 2022 11:21:09 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > On 2022/07/21 10:13, Masahiko Sawada wrote: > > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > describe subtransaction XIDs. I've attached the patch to improve the > > description. > > +1 > > The patch looks good to me. The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit also shows subtransactions but they are at maximum 64. I feel like -0.3 if there's no obvious advantage showing them. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Jul 21, 2022 at 4:29 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 21 Jul 2022 11:21:09 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > > > > On 2022/07/21 10:13, Masahiko Sawada wrote: > > > Hi, > > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > > describe subtransaction XIDs. I've attached the patch to improve the > > > description. > > > > +1 > > > > The patch looks good to me. > > The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = > PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit > also shows subtransactions but they are at maximum 64. I feel like > -0.3 if there's no obvious advantage showing them. xxx_desc() functions are debugging purpose functions as they are used by pg_waldump and pg_walinspect etc. I think such functions should show all contents unless there is reason to hide them. Particularly, standby_desc_running_xacts() currently shows subtransaction information only when subtransactions are overflowed, which got me confused when inspecting WAL records. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
At Thu, 21 Jul 2022 16:58:45 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > > The patch looks good to me. By the way +1 to this. > > The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = > > PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit > > also shows subtransactions but they are at maximum 64. I feel like > > -0.3 if there's no obvious advantage showing them. > > xxx_desc() functions are debugging purpose functions as they are used > by pg_waldump and pg_walinspect etc. I think such functions should > show all contents unless there is reason to hide them. Particularly, > standby_desc_running_xacts() currently shows subtransaction > information only when subtransactions are overflowed, which got me > confused when inspecting WAL records. I'm not sure just confusing can justify that but after finding logicalmsg_desc dumps the whole content, I no longer feel against to show subxacts. Just for information, but as far as I saw, relmap_desc shows only the length of "data" but doesn't dump all of it. generic_desc behaves the same way. Thus we could just show "%d subxacts" instead of dumping out the all subxact ids just to avoid that confusion. However, again, I no longer object to show all subxids. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > describe subtransaction XIDs. I've attached the patch to improve the > description. Here is an example by pg_wlaldump: > > * HEAD > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 > > * w/ patch > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 > subxacts: 1049 > I think this is a good addition to debugging info. +1 If we are going to add 64 subxid numbers then it would help if we could be more verbose and print "subxid overflowed" instead of "subxid ovf". -- Best Wishes, Ashutosh Bapat
On Thu, Jul 21, 2022 at 10:13 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > Hi > > On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > Hi, > > > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > describe subtransaction XIDs. I've attached the patch to improve the > > description. Here is an example by pg_wlaldump: > > > > * HEAD > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 > > > > * w/ patch > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 > > subxacts: 1049 > > > > I think this is a good addition to debugging info. +1 > > If we are going to add 64 subxid numbers then it would help if we > could be more verbose and print "subxid overflowed" instead of "subxid > ovf". Yeah, it looks better so agreed. I've attached an updated patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Attachment
Thanks Masahiko for the updated patch. It looks good to me.
I wonder whether the logic should be, similar to ProcArrayApplyRecoveryInfo()
if (xlrec->subxid_overflow)
...
else if (xlrec->subxcnt > 0)
...
But you may ignore it.
Best Wishes,
Ashutosh
On Thu, Jul 28, 2022 at 7:41 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Jul 21, 2022 at 10:13 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi
>
> On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Hi,
> >
> > I realized that standby_desc_running_xacts() in standbydesc.c doesn't
> > describe subtransaction XIDs. I've attached the patch to improve the
> > description. Here is an example by pg_wlaldump:
> >
> > * HEAD
> > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn:
> > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050
> > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048
> >
> > * w/ patch
> > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn:
> > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050
> > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1
> > subxacts: 1049
> >
>
> I think this is a good addition to debugging info. +1
>
> If we are going to add 64 subxid numbers then it would help if we
> could be more verbose and print "subxid overflowed" instead of "subxid
> ovf".
Yeah, it looks better so agreed. I've attached an updated patch.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
At Thu, 28 Jul 2022 09:56:33 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in
> Thanks Masahiko for the updated patch. It looks good to me.
>
> I wonder whether the logic should be, similar
> to ProcArrayApplyRecoveryInfo()
> if (xlrec->subxid_overflow)
> ...
> else if (xlrec->subxcnt > 0)
> ...
>
> But you may ignore it.
Either is fine if we asuume the record is sound, but since it is
debugging output, I think we should always output the information *for
both* . The following change doesn't change the output for a sound
record.
====
if (xlrec->subxcnt > 0)
{
appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
for (i = 0; i < xlrec->subxcnt; i++)
appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
}
- else if (xlrec->subxid_overflow)
+ if (xlrec->subxid_overflow)
appendStringInfoString(buf, "; subxid overflowed");
====
Another point is if the xid/subxid lists get long, I see it annoying
that the "overflowed" messages goes far away to the end of the long
line. Couldn't we rearrange the item order of the line as the follows?
nextXid %u latestCompletedXid %u oldestRunningXid %u;[ subxid overflowed;][ %d xacts: %u %u ...;][ subxacts: %u %u ..]
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Jul 28, 2022 at 3:24 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 28 Jul 2022 09:56:33 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in
> > Thanks Masahiko for the updated patch. It looks good to me.
> >
> > I wonder whether the logic should be, similar
> > to ProcArrayApplyRecoveryInfo()
> > if (xlrec->subxid_overflow)
> > ...
> > else if (xlrec->subxcnt > 0)
> > ...
> >
> > But you may ignore it.
>
> Either is fine if we asuume the record is sound, but since it is
> debugging output, I think we should always output the information *for
> both* . The following change doesn't change the output for a sound
> record.
>
> ====
> if (xlrec->subxcnt > 0)
> {
> appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
> for (i = 0; i < xlrec->subxcnt; i++)
> appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
> }
> - else if (xlrec->subxid_overflow)
> + if (xlrec->subxid_overflow)
> appendStringInfoString(buf, "; subxid overflowed");
> ====
Do you mean that both could be true at the same time? If I read
GetRunningTransactionData() correctly, that doesn't happen.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
At Thu, 28 Jul 2022 15:53:33 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > > Do you mean that both could be true at the same time? If I read > GetRunningTransactionData() correctly, that doesn't happen. So, I wrote "since it is debugging output", and "fine if we asuume the record is sound". Is it any trouble with assuming the both *can* happen at once? If something's broken, it will be reflected in the output. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry for the late reply.
On Thu, Jul 28, 2022 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 28 Jul 2022 15:53:33 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > >
> > Do you mean that both could be true at the same time? If I read
> > GetRunningTransactionData() correctly, that doesn't happen.
>
> So, I wrote "since it is debugging output", and "fine if we asuume the
> record is sound". Is it any trouble with assuming the both *can*
> happen at once? If something's broken, it will be reflected in the
> output.
Fair point. We may not need to interpret the contents.
On Thu, Jul 28, 2022 at 3:24 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Another point is if the xid/subxid lists get long, I see it annoying
> that the "overflowed" messages goes far away to the end of the long
> line. Couldn't we rearrange the item order of the line as the follows?
>
> nextXid %u latestCompletedXid %u oldestRunningXid %u;[ subxid overflowed;][ %d xacts: %u %u ...;][ subxacts: %u %u
..]
>
I'm concerned that we have two information of subxact apart. Given
that showing both individual subxacts and "overflow" is a bug, I think
we can output like:
if (xlrec->subxcnt > 0)
{
appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
for (i = 0; i < xlrec->subxcnt; i++)
appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
}
if (xlrec->subxid_overflow)
appendStringInfoString(buf, "; subxid overflowed");
Or we can output the "subxid overwlowed" first.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
At Mon, 15 Aug 2022 11:16:56 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> Sorry for the late reply.
No worries. Anyway I was in a long (as a Japanese:) vacation.
> On Thu, Jul 28, 2022 at 4:29 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > record is sound". Is it any trouble with assuming the both *can*
> > happen at once? If something's broken, it will be reflected in the
> > output.
>
> Fair point. We may not need to interpret the contents.
Yeah.
> On Thu, Jul 28, 2022 at 3:24 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Another point is if the xid/subxid lists get long, I see it annoying
> > that the "overflowed" messages goes far away to the end of the long
> > line. Couldn't we rearrange the item order of the line as the follows?
> >
> > nextXid %u latestCompletedXid %u oldestRunningXid %u;[ subxid overflowed;][ %d xacts: %u %u ...;][ subxacts: %u %u
..]
> >
>
> I'm concerned that we have two information of subxact apart. Given
> that showing both individual subxacts and "overflow" is a bug, I think
Bug or every kind of breakage of the file. So if "overflow"ed, we
don't need to show a subxid there but I thought that there's no need
to change behavior in that case since it scarcely happens.
> we can output like:
>
> if (xlrec->subxcnt > 0)
> {
> appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
> for (i = 0; i < xlrec->subxcnt; i++)
> appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
> }
>
> if (xlrec->subxid_overflow)
> appendStringInfoString(buf, "; subxid overflowed");
Yea, it seems like what I proposed upthread. I'm fine with that since
it is an abonormal situation.
> Or we can output the "subxid overwlowed" first.
(I prefer this, as that doesn't change the output in the normal case
but the anormality will be easilly seen if happens.)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Tue, Aug 23, 2022 at 11:53 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 15 Aug 2022 11:16:56 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > Sorry for the late reply.
>
> No worries. Anyway I was in a long (as a Japanese:) vacation.
>
> > On Thu, Jul 28, 2022 at 4:29 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > > record is sound". Is it any trouble with assuming the both *can*
> > > happen at once? If something's broken, it will be reflected in the
> > > output.
> >
> > Fair point. We may not need to interpret the contents.
>
> Yeah.
>
> > On Thu, Jul 28, 2022 at 3:24 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > Another point is if the xid/subxid lists get long, I see it annoying
> > > that the "overflowed" messages goes far away to the end of the long
> > > line. Couldn't we rearrange the item order of the line as the follows?
> > >
> > > nextXid %u latestCompletedXid %u oldestRunningXid %u;[ subxid overflowed;][ %d xacts: %u %u ...;][ subxacts: %u
%u..]
> > >
> >
> > I'm concerned that we have two information of subxact apart. Given
> > that showing both individual subxacts and "overflow" is a bug, I think
>
> Bug or every kind of breakage of the file. So if "overflow"ed, we
> don't need to show a subxid there but I thought that there's no need
> to change behavior in that case since it scarcely happens.
>
> > we can output like:
> >
> > if (xlrec->subxcnt > 0)
> > {
> > appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
> > for (i = 0; i < xlrec->subxcnt; i++)
> > appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
> > }
> >
> > if (xlrec->subxid_overflow)
> > appendStringInfoString(buf, "; subxid overflowed");
>
> Yea, it seems like what I proposed upthread. I'm fine with that since
> it is an abonormal situation.
>
> > Or we can output the "subxid overwlowed" first.
>
> (I prefer this, as that doesn't change the output in the normal case
> but the anormality will be easilly seen if happens.)
>
Updated the patch accordingly.
Regards,
--
Masahiko Sawada
Attachment
At Fri, 9 Sep 2022 09:48:05 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> On Tue, Aug 23, 2022 at 11:53 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Mon, 15 Aug 2022 11:16:56 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > > Or we can output the "subxid overwlowed" first.
> >
> > (I prefer this, as that doesn't change the output in the normal case
> > but the anormality will be easilly seen if happens.)
> >
>
> Updated the patch accordingly.
Thanks! Considering the discussion so far, how about adding a comment
like this?
+ appendStringInfoString(buf, "; subxid overflowed");
+
++ /*
++ * subxids and subxid_overflow are mutually exclusive, but we deliberitely
++ * print the both simultaneously in case the record is broken.
++ */
+ if (xlrec->subxcnt > 0)
+ {
+ appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
+ for (i = 0; i < xlrec->subxcnt; i++)
+ appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
+ }
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Sep 9, 2022 at 6:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Updated the patch accordingly. > I have created two xacts each with savepoints and after your patch, the record will show xacts/subxacts information as below: rmgr: Standby len (rec/tot): 74/ 74, tx: 0, lsn: 0/014AC238, prev 0/014AC1F8, desc: RUNNING_XACTS nextXid 733 latestCompletedXid 726 oldestRunningXid 727; 2 xacts: 729 727; 4 subxacts: 730 731 728 732 There is no way to associate which subxacts belong to which xact, so will it be useful, and if so, how? I guess we probably don't need it here because the describe records just display the record information. -- With Regards, Amit Kapila.
Hi, When translating error messages, Alexander Lakhin (<exclusion@gmail.com>) noticed some inconsistencies so I prepared a small patch to fix those. Please see attached. -- Ekaterina Kiryanova Technical Writer Postgres Professional the Russian PostgreSQL Company
Attachment
On Wed, Sep 14, 2022 at 5:01 PM Ekaterina Kiryanova
<e.kiryanova@postgrespro.ru> wrote:
>
> Hi,
>
> When translating error messages, Alexander Lakhin
> (<exclusion@gmail.com>) noticed some inconsistencies so I prepared a
> small patch to fix those.
+1
This one
- errmsg("background worker \"%s\": background worker without shared
memory access are not supported",
+ errmsg("background worker \"%s\": background workers without shared
memory access are not supported",
is a grammar error so worth backpatching, but the rest are cosmetic.
Will commit this way unless there are objections.
--
John Naylor
EDB: http://www.enterprisedb.com
On 2022-Sep-14, John Naylor wrote:
> This one
>
> + errmsg("background worker \"%s\": background workers without shared
> memory access are not supported",
>
> is a grammar error so worth backpatching, but the rest are cosmetic.
>
> Will commit this way unless there are objections.
+1
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)
On Wed, Sep 14, 2022 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Sep 9, 2022 at 6:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > Updated the patch accordingly. > > > > I have created two xacts each with savepoints and after your patch, > the record will show xacts/subxacts information as below: > > rmgr: Standby len (rec/tot): 74/ 74, tx: 0, lsn: > 0/014AC238, prev 0/014AC1F8, desc: RUNNING_XACTS nextXid 733 > latestCompletedXid 726 oldestRunningXid 727; 2 xacts: 729 727; 4 > subxacts: 730 731 728 732 > > There is no way to associate which subxacts belong to which xact, so > will it be useful, and if so, how? I guess we probably don't need it > here because the describe records just display the record information. I think it's useful for debugging purposes. For instance, when I was working on the fix 68dcce247f1a13318613a0e27782b2ca21a4ceb7 (REL_14_STABLE), I checked if all initial running transactions including subtransactions are properly stored and purged by checking the debug logs and pg_waldump output. Actually, until I realize that the description of RUNNING_XACTS doesn't show subtransaction information, I was confused by the fact that the saved initial running transactions didn't match the description shown by pg_waldump. Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Sep 15, 2022 at 1:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Sep 14, 2022 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Fri, Sep 9, 2022 at 6:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > Updated the patch accordingly. > > > > > > > I have created two xacts each with savepoints and after your patch, > > the record will show xacts/subxacts information as below: > > > > rmgr: Standby len (rec/tot): 74/ 74, tx: 0, lsn: > > 0/014AC238, prev 0/014AC1F8, desc: RUNNING_XACTS nextXid 733 > > latestCompletedXid 726 oldestRunningXid 727; 2 xacts: 729 727; 4 > > subxacts: 730 731 728 732 > > > > There is no way to associate which subxacts belong to which xact, so > > will it be useful, and if so, how? I guess we probably don't need it > > here because the describe records just display the record information. > > I think it's useful for debugging purposes. For instance, when I was > working on the fix 68dcce247f1a13318613a0e27782b2ca21a4ceb7 > (REL_14_STABLE), I checked if all initial running transactions > including subtransactions are properly stored and purged by checking > the debug logs and pg_waldump output. Actually, until I realize that > the description of RUNNING_XACTS doesn't show subtransaction > information, I was confused by the fact that the saved initial running > transactions didn't match the description shown by pg_waldump. > I see your point but I am still worried due to the concern raised by Horiguchi-San earlier in this thread that the total number could be as large as TOTAL_MAX_CACHED_SUBXIDS. I think if we want to include information only on the number of subxacts then that is clearly an improvement without any disadvantage. Does anyone else have an opinion on this matter? -- With Regards, Amit Kapila.
At Thu, 15 Sep 2022 17:39:17 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in > I see your point but I am still worried due to the concern raised by > Horiguchi-San earlier in this thread that the total number could be as > large as TOTAL_MAX_CACHED_SUBXIDS. I think if we want to include > information only on the number of subxacts then that is clearly an > improvement without any disadvantage. > > Does anyone else have an opinion on this matter? The doesn't seem to work for Sawada-san's case, but I'm fine with that:p Putting an arbitrary upper-bound on the number of subxids to print might work? I'm not sure how we can determine the upper-bound, though. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Sep 14, 2022 at 5:25 PM John Naylor <john.naylor@enterprisedb.com> wrote: > Will commit this way unless there are objections. I forgot to mention yesterday, but this is done. -- John Naylor EDB: http://www.enterprisedb.com
On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > Putting an arbitrary upper-bound on the number of subxids to print > might work? I'm not sure how we can determine the upper-bound, though. You could hardcode it so as it does not blow up the whole view, say 20~30. Anyway, I agree with the concern raised upthread about the amount of extra data this would add to the output, so having at least the number of subxids would be better than the existing state of things telling only if the list of overflowed. So let's stick to that. Here is another idea for the list of subxids: show the full list of subxids only when using --xid. We could always introduce an extra switch, but that does not seem worth the extra workload here. -- Michael
Attachment
On Mon, Oct 3, 2022 at 5:15 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > > Putting an arbitrary upper-bound on the number of subxids to print > > might work? I'm not sure how we can determine the upper-bound, though. > > You could hardcode it so as it does not blow up the whole view, say > 20~30. Anyway, I agree with the concern raised upthread about the > amount of extra data this would add to the output, so having at least > the number of subxids would be better than the existing state of > things telling only if the list of overflowed. So let's stick to > that. Why are only subtransaction information in XLOG_RUNNING_XACTS limited? I think we have other information shown without bounds such as lock information in XLOG_STANDBY_LOCK and invalidation messages in XLOG_INVALIDATIONS. Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Mon, Oct 3, 2022 at 1:46 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > > Putting an arbitrary upper-bound on the number of subxids to print > > might work? I'm not sure how we can determine the upper-bound, though. > > You could hardcode it so as it does not blow up the whole view, say > 20~30. Anyway, I agree with the concern raised upthread about the > amount of extra data this would add to the output, so having at least > the number of subxids would be better than the existing state of > things telling only if the list of overflowed. So let's stick to > that. I spent some time today reading this. As others said upthread, the output can be more verbose if all the backends are running max subtransactions or subtransactions overflow occurred in all the backends. This can blow-up the output. Hard-limiting the number of subxids isn't a good idea because the whole purpose of it is gone. As Amit said upthread, we can't really link subtxns with the corresponding txns by looking at the output of the pg_waldump. And I understand that having the subxid info helped Mashaiko-san debug an issue. Wouldn't it be better to have a SQL-callable function that can return txn and all its subxid information? I'm not sure if it's useful or worth at all because the contents are so dynamic. I'm not sure if we have one already or if it's possible to have one such function. > Here is another idea for the list of subxids: show the full list of > subxids only when using --xid. We could always introduce an extra > switch, but that does not seem worth the extra workload here. This seems interesting, but I agree that the extra code isn't worth it. FWIW, I quickly looked at few other resource managers XXXX_desc functions to find if they output all the record's info: xlog_desc - doesn't show restart point timestamp for xl_restore_point record type and logicalmsg_desc - doesn't show the database id that generated the record clog_desc - doesn't show oldest xact db of xl_clog_truncate record and there may be more. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Oct 14, 2022 at 3:38 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, Oct 3, 2022 at 1:46 PM Michael Paquier <michael@paquier.xyz> wrote: > > > > On Fri, Sep 16, 2022 at 10:55:53AM +0900, Kyotaro Horiguchi wrote: > > > Putting an arbitrary upper-bound on the number of subxids to print > > > might work? I'm not sure how we can determine the upper-bound, though. > > > > You could hardcode it so as it does not blow up the whole view, say > > 20~30. Anyway, I agree with the concern raised upthread about the > > amount of extra data this would add to the output, so having at least > > the number of subxids would be better than the existing state of > > things telling only if the list of overflowed. So let's stick to > > that. > > I spent some time today reading this. As others said upthread, the > output can be more verbose if all the backends are running max > subtransactions or subtransactions overflow occurred in all the > backends. > As far as I can understand, this contains subtransactions only when they didn't overflow. The latest information provided by Sawada-San for similar records (XLOG_STANDBY_LOCK and XLOG_INVALIDATIONS) made me think that maybe we are just over-worried about the worst case. > This can blow-up the output. > If we get some reports like that, then we can probably use Michael's idea of displaying additional information with a separate flag. > Hard-limiting the number of subxids isn't a good idea because the > whole purpose of it is gone. > Agreed. -- With Regards, Amit Kapila.
On Sat, Oct 15, 2022 at 4:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > I spent some time today reading this. As others said upthread, the
> > output can be more verbose if all the backends are running max
> > subtransactions or subtransactions overflow occurred in all the
> > backends.
> >
>
> As far as I can understand, this contains subtransactions only when
> they didn't overflow. The latest information provided by Sawada-San
> for similar records (XLOG_STANDBY_LOCK and XLOG_INVALIDATIONS) made me
> think that maybe we are just over-worried about the worst case.
Agreed. I see the below comment, which means when
xlrec->subxid_overflow is set to true, there will not be any
subtransaction ids logged in the WAL record.
* Note that if any transaction has overflowed its cached subtransactions
* then there is no real need include any subtransactions.
*/
RunningTransactions
GetRunningTransactionData(void)
If my above understanding is correct, having something like below does
no harm, like Masahiko-san's one of the initial patches, no? I'm also
fine with the way it is in the v3 patch.
if (xlrec->subxid_overflow)
{
/*
* Server doesn't include any subtransactions if any transaction has
* overflowed its cached subtransactions.
*/
Assert(xlrec->subxcnt == 0)
appendStringInfoString(buf, "; subxid overflowed");
}
else if (xlrec->subxcnt > 0)
{
appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt);
for (i = 0; i < xlrec->subxcnt; i++)
appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]);
}
The v3 patch posted upthread LGTM and I marked it as RfC. I'm just
reattaching the v3 patch posted upthread herewith so that the cfbot
can test the right patch - https://commitfest.postgresql.org/40/3779/.
> >
> This can blow-up the output.
> >
>
> If we get some reports like that, then we can probably use Michael's
> idea of displaying additional information with a separate flag.
Agreed.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment
At Sun, 16 Oct 2022 12:32:56 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Sat, Oct 15, 2022 at 4:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > I spent some time today reading this. As others said upthread, the > > > output can be more verbose if all the backends are running max > > > subtransactions or subtransactions overflow occurred in all the > > > backends. > > > > > > > As far as I can understand, this contains subtransactions only when > > they didn't overflow. The latest information provided by Sawada-San > > for similar records (XLOG_STANDBY_LOCK and XLOG_INVALIDATIONS) made me > > think that maybe we are just over-worried about the worst case. > > Agreed. I see the below comment, which means when > xlrec->subxid_overflow is set to true, there will not be any > subtransaction ids logged in the WAL record. Since I categorized this tool as semi-debugging purpose so I'm fine that sometimes very long lines are seen. In the first place it is already seen in, for example, transaction commit records. They can be 30k characters long by many relfile locators, stats locators, invalidations and snapshots, when 100 relations are dropped. > If my above understanding is correct, having something like below does > no harm, like Masahiko-san's one of the initial patches, no? I'm also > fine with the way it is in the v3 patch. Yeah, v3 works exactly the same way with the initial patch, except when something bad happens in that record. So *I* thought that it's rather better that the tool describes records as-is (even if only for this record..:p) rather than how the broken records are recognized by the recovery code. > The v3 patch posted upthread LGTM and I marked it as RfC. I'm just > reattaching the v3 patch posted upthread herewith so that the cfbot > can test the right patch - https://commitfest.postgresql.org/40/3779/. > > > > > > This can blow-up the output. > > > > > > > If we get some reports like that, then we can probably use Michael's > > idea of displaying additional information with a separate flag. > > Agreed. Agreed, but maybe we need to recheck what should be hidden (or abbreviated) in the concise (or terse?) mode. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Oct 17, 2022 at 6:46 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Sun, 16 Oct 2022 12:32:56 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > > On Sat, Oct 15, 2022 at 4:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > I spent some time today reading this. As others said upthread, the > > > > output can be more verbose if all the backends are running max > > > > subtransactions or subtransactions overflow occurred in all the > > > > backends. > > > > > > > > > > As far as I can understand, this contains subtransactions only when > > > they didn't overflow. The latest information provided by Sawada-San > > > for similar records (XLOG_STANDBY_LOCK and XLOG_INVALIDATIONS) made me > > > think that maybe we are just over-worried about the worst case. > > > > Agreed. I see the below comment, which means when > > xlrec->subxid_overflow is set to true, there will not be any > > subtransaction ids logged in the WAL record. > > Since I categorized this tool as semi-debugging purpose so I'm fine > that sometimes very long lines are seen. In the first place it is > already seen in, for example, transaction commit records. They can be > 30k characters long by many relfile locators, stats locators, > invalidations and snapshots, when 100 relations are dropped. > > > If my above understanding is correct, having something like below does > > no harm, like Masahiko-san's one of the initial patches, no? I'm also > > fine with the way it is in the v3 patch. > > Yeah, v3 works exactly the same way with the initial patch, except > when something bad happens in that record. So *I* thought that it's > rather better that the tool describes records as-is (even if only for > this record..:p) rather than how the broken records are recognized by > the recovery code. > Okay, let's wait for two or three days and see if anyone thinks differently, otherwise, I'll push v3 after a bit more testing. -- With Regards, Amit Kapila.
On Mon, Oct 17, 2022 at 09:58:31AM +0530, Amit Kapila wrote: > Okay, let's wait for two or three days and see if anyone thinks > differently, otherwise, I'll push v3 after a bit more testing. No objections from here if you want to go ahead with v3 and print the full set of subxids on top of the information about these overflowing. -- Michael
Attachment
On Mon, Oct 17, 2022 at 02:53:57PM +0900, Michael Paquier wrote: > No objections from here if you want to go ahead with v3 and print the > full set of subxids on top of the information about these > overflowing. While browsing the CF entries, this was still listed. Amit, any updates? -- Michael
Attachment
On Tue, Nov 1, 2022 at 12:53 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Oct 17, 2022 at 02:53:57PM +0900, Michael Paquier wrote: > > No objections from here if you want to go ahead with v3 and print the > > full set of subxids on top of the information about these > > overflowing. > > While browsing the CF entries, this was still listed. Amit, any > updates? > I am planning to take care of this entry sometime this week. -- With Regards, Amit Kapila.
On Tue, Nov 1, 2022 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Nov 1, 2022 at 12:53 PM Michael Paquier <michael@paquier.xyz> wrote: > > > > On Mon, Oct 17, 2022 at 02:53:57PM +0900, Michael Paquier wrote: > > > No objections from here if you want to go ahead with v3 and print the > > > full set of subxids on top of the information about these > > > overflowing. > > > > While browsing the CF entries, this was still listed. Amit, any > > updates? > > > > I am planning to take care of this entry sometime this week. > Pushed. -- With Regards, Amit Kapila.
2022年11月2日(水) 15:24 Amit Kapila <amit.kapila16@gmail.com>: > > On Tue, Nov 1, 2022 at 4:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Nov 1, 2022 at 12:53 PM Michael Paquier <michael@paquier.xyz> wrote: > > > > > > On Mon, Oct 17, 2022 at 02:53:57PM +0900, Michael Paquier wrote: > > > > No objections from here if you want to go ahead with v3 and print the > > > > full set of subxids on top of the information about these > > > > overflowing. > > > > > > While browsing the CF entries, this was still listed. Amit, any > > > updates? > > > > > > > I am planning to take care of this entry sometime this week. > > > > Pushed. Marked as committed in the CF app, many thanks for closing this one out. Regards Ian Barwick