public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: 'vignesh C' <[email protected]>
Cc: Greg Nancarrow <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: Failed transaction statistics to measure the logical replication progress
Date: Mon, 15 Nov 2021 09:31:20 +0000
Message-ID: <TYCPR01MB83730FAF8FB392844524647CED989@TYCPR01MB8373.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO+KW3O9LX7X6b8mr8A@mail.gmail.com>
References: <TYCPR01MB8373DC922E549FF3F031E9DAED8E9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CAJcOf-cMU_1VJ1c+9fwEBgimkj+=hHcCjj_SUfbHvKTuN=1tuw@mail.gmail.com>
<TYCPR01MB8373B1614522B90BA96883E0ED929@TYCPR01MB8373.jpnprd01.prod.outlook.com>
<CALDaNm3-QTZnd4rxbrUfKMM4u_vgb23LO+KW3O9LX7X6b8mr8A@mail.gmail.com>
On Wednesday, November 10, 2021 7:13 PM vignesh C <[email protected]> wrote:
> On Tue, Nov 9, 2021 at 5:05 PM [email protected]
> <[email protected]> wrote:
> > Yes. I've rebased and updated the patch, paying attention to this point.
> > Attached the updated version.
>
> Thanks for the updated patch, few comments:
> 1) you could rename PgStat_StatSubWorkerPreparedXact to
> PgStat_SW_PreparedXactKey or a simpler name which includes key and
> similarly change PgStat_StatSubWorkerPreparedXactSize to
> PgStat_SW_PreparedXactEntry
>
> +/* prepared transaction */
> +typedef struct PgStat_StatSubWorkerPreparedXact {
> + Oid subid;
> + char gid[GIDSIZE];
> +} PgStat_StatSubWorkerPreparedXact;
> +
> +typedef struct PgStat_StatSubWorkerPreparedXactSize
> +{
> + PgStat_StatSubWorkerPreparedXact key; /* hash key */
> +
> + Oid subid;
> + char gid[GIDSIZE];
> + PgStat_Counter xact_size;
> +} PgStat_StatSubWorkerPreparedXactSize;
> +
Fixed. Adopted your suggested names.
> 2) You can change prepared_size to sw_prepared_xact_entry or
> prepared_xact_entry since it is a hash entry with few fields
> + if (subWorkerPreparedXactSizeHash)
> + {
> + PgStat_StatSubWorkerPreparedXactSize *prepared_size;
> +
> + hash_seq_init(&hstat, subWorkerPreparedXactSizeHash);
> + while((prepared_size =
> (PgStat_StatSubWorkerPreparedXactSize *) hash_seq_search(&hstat)) !=
> NULL)
> + {
> + fputc('P', fpout);
> + rc = fwrite(prepared_size,
> sizeof(PgStat_StatSubWorkerPreparedXactSize), 1, fpout);
> + (void) rc; /* we'll check
> for error with ferror */
> + }
I preferred prepared_xact_entry. Fixed.
> 3) This need to be indented
> - w.relid,
> - w.command,
> - w.xid,
> - w.error_count,
> - w.error_message,
> - w.last_error_time
> + w.commit_count,
> + w.commit_bytes,
> + w.error_count,
> + w.error_bytes,
> + w.abort_count,
> + w.abort_bytes,
> + w.last_error_relid,
> + w.last_error_command,
> + w.last_error_xid,
> + w.last_error_count,
> + w.last_error_message,
> + w.last_error_time
Fixed.
> 4) Instead of adding a function to calculate the size, can we move
> PartitionTupleRouting from c file to the header file and use sizeof at the caller
> function?
> +/*
> + * PartitionTupleRoutingSize - exported to calculate total data size
> + * of logical replication mesage apply, because this is one of the
> + * ApplyExecutionData struct members.
> + */
> +size_t
> +PartitionTupleRoutingSize(void)
> +{
> + return sizeof(PartitionTupleRouting); }
Fixed.
> 5) You could run pgindent and pgperltidy for the code and test code to fix the
> indent issues.
> +
> subWorkerPreparedXactSizeHash = hash_create("Subscription worker stats
> of prepared txn",
> +
>
> PGSTAT_SUBWORKER_HASH_SIZE,
> +
> &hash_ctl,
> +
> HASH_ELEM | HASH_STRINGS |
> HASH_CONTEXT);
> +# There's no entry at the beginning
> +my $result = $node_subscriber->safe_psql('postgres',
> +"SELECT count(*) FROM pg_stat_subscription_workers;"); is($result,
> +q(0), 'no entry for transaction stats yet');
Conducted pgindent and pgperltidy.
> 6) Few places you have used strlcpy and few places you have used memcpy,
> you can keep it consistent:
> + msg.m_command = command;
> + strlcpy(msg.m_gid, gid, sizeof(msg.m_gid));
> + msg.m_xact_bytes = xact_size;
>
> + key.subid = subid;
> + memcpy(key.gid, gid, sizeof(key.gid));
> + action = (create ? HASH_ENTER : HASH_FIND);
Fixed. I used strlcpy for new additional functions I made.
An exception is pgstat_read_db_statsfile().
In this function, we use memcpy() consistently in other places.
Please have a look at [1]
[1] - https://www.postgresql.org/message-id/TYCPR01MB8373FEB287F733C81C1E4D42ED989%40TYCPR01MB8373.jpnprd0...
Best Regards,
Takamichi Osumi
view thread (113+ 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]
Subject: RE: Failed transaction statistics to measure the logical replication progress
In-Reply-To: <TYCPR01MB83730FAF8FB392844524647CED989@TYCPR01MB8373.jpnprd01.prod.outlook.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