Обсуждение: Transaction commits VS Transaction commits (with parallel) VS querymean time

On Fri, Feb 8, 2019 at 6:55 AM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
> On Thu, Feb 7, 2019 at 9:31 PM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>>
>>
>> This is because of larger xact_commit value than default configuration. With the changed server configuration, that
leadsto generate more parallel workers and every parallel worker operation is treated as an extra commit, because of
thisreason, the total number of commits increased, but the overall query performance is decreased. 
>>
>> Is there any relation of transaction commits to performance?
>>
>> Is there any specific reason to consider the parallel worker activity also as a transaction commit? Especially in my
observation,if we didn't consider the parallel worker activity as separate commits, the test doesn't show an increase
intransaction commits. 
>
>
> The following statements shows the increase in the xact_commit value with
> parallel workers. I can understand that workers updating the seq_scan stats
> as they performed the seq scan.
>

Yeah, that seems okay, however, one can say that for the scan they
want to consider it as a single scan even if part of the scan is
accomplished by workers or may be a separate counter for parallel
workers scan.

> Is the same applied to parallel worker transaction
> commits also?
>

I don't think so.  It seems to me that we should consider it as a
single transaction.  Do you want to do the leg work for this and try
to come up with a patch?  On a quick look, I think we might want to
change AtEOXact_PgStat so that the commits for parallel workers are
not considered.  I think the caller has that information.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Feb 10, 2019 at 10:54 AM Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Sat, Feb 9, 2019 at 4:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> I don't think so.  It seems to me that we should consider it as a
>> single transaction.  Do you want to do the leg work for this and try
>> to come up with a patch?  On a quick look, I think we might want to
>> change AtEOXact_PgStat so that the commits for parallel workers are
>> not considered.  I think the caller has that information.
>
>
> I try to fix it by adding a check for parallel worker or not and based on it
> count them into stats. Patch attached.
>

@@ -2057,14 +2058,18 @@ AtEOXact_PgStat(bool isCommit)
 {
..
+ /* Don't count parallel worker transactions into stats */
+ if (!IsParallelWorker())
+ {
+ /*
+ * Count transaction commit or abort.  (We use counters, not just bools,
+ * in case the reporting message isn't sent right away.)
+ */
+ if (isCommit)
+ pgStatXactCommit++;
+ else
+ pgStatXactRollback++;
+ }
..
}

I wonder why you haven't used the 'is_parallel_worker' flag from the
caller,  see CommitTransaction/AbortTransaction?  The difference is
that if we use that then it will just avoid counting transaction for
the parallel work (StartParallelWorkerTransaction), otherwise, it
might miss the count of any other transaction we started in the
parallel worker for some intermediate work (for example, the
transaction we started to restore library and guc state).

I think it boils down to whether we want to avoid any transaction that
is started by a parallel worker or just the transaction which is
shared among leader and worker.  It seems to me that currently, we do
count all the internal transactions (started with
StartTransactionCommand and CommitTransactionCommand) in this counter,
so we should just try to avoid the transaction for which state is
shared between leader and workers.

Anyone else has any opinion on this matter?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 21, 2019 at 2:18 AM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> With Inclusion of parallel worker transactions, the TPS will be a higher number,
> thus user may find it as better throughput. This is the case with one of our tool.
> The tool changes the configuration randomly to find out the best configuration
> for the server based on a set of workload, during our test, with some configurations,
> the TPS is so good, but the overall performance is slow as the system is having
> less resources to keep up with that configuration.
>
> Opinions?

Well, I think that might be a sign that the data isn't being used
correctly.  I don't have a strong position on what the "right" thing
to do here is, but I think if you want to know how many client
transactions are being executed, you should count them on the client
side, as pgbench does.  I agree that it's a little funny to count the
parallel worker commit as a separate transaction, since in a certain
sense it is part of the same transaction.  But if you do that, then as
already noted you have to next decide what to do about other
transactions that parallel workers use internally.  And then you have
to decide what to do about other background transactions.  And there's
not really one "right" answer to any of these questions, I don't
think.  You might want to count different things depending on how the
information is going to be used.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2019-Mar-21, Robert Haas wrote:

> I don't have a strong position on what the "right" thing
> to do here is, but I think if you want to know how many client
> transactions are being executed, you should count them on the client
> side, as pgbench does.

I think counting on the client side is untenable in practical terms.
They may not even know what clients there are, or may not have the
source code to add such a feature even if they know how.  Plus they
would have to aggregate data coming from dozens of different systems?
I don't think this argument has any wings.

OTOH the reason the server offers stats is so that the user can know
what the server activity is, not to display useless internal state.
If a user disables an internal option (such as parallel query) and their
monitoring system suddenly starts showing half as many transactions as
before, they're legitimaly going to think that the server is broken.
Such stats are pointless.

The use case for those stats seems fairly clear to me: display numbers
in a monitoring system.  You seem to be saying that we're just not going
to help developers of monitoring systems, and that users have to feed
them on their own.

> I agree that it's a little funny to count the parallel worker commit
> as a separate transaction, since in a certain sense it is part of the
> same transaction.

Right.  So you don't count it.  This seems crystal clear.  Doing the
other thing is outright wrong, there's no doubt about that.

> But if you do that, then as already noted you have to next decide what
> to do about other transactions that parallel workers use internally.

You don't count those ones either.

> And then you have to decide what to do about other background
> transactions.

Not count them if they're implementation details; otherwise count them.
For example, IMO autovacuum transactions should definitely be counted
(as one transaction, even if they run parallel vacuum).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Mar 22, 2019 at 12:34 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> > And then you have to decide what to do about other background
> > transactions.
>
> Not count them if they're implementation details; otherwise count them.
> For example, IMO autovacuum transactions should definitely be counted
> (as one transaction, even if they run parallel vacuum).

Hmm, interesting.  autovacuum isn't an implementation detail?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 22, 2019 at 10:04 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2019-Mar-21, Robert Haas wrote:
>
> > I agree that it's a little funny to count the parallel worker commit
> > as a separate transaction, since in a certain sense it is part of the
> > same transaction.
>
> Right.  So you don't count it.  This seems crystal clear.  Doing the
> other thing is outright wrong, there's no doubt about that.
>

Agreed, this is a clear problem and we can just fix this and move
ahead, but it is better if we spend some more time to see if we can
come up with something better.  We might want to just fix this and
then deal with the second part of the problem separately along with
some other similar cases.

> > But if you do that, then as already noted you have to next decide what
> > to do about other transactions that parallel workers use internally.
>
> You don't count those ones either.
>

Yeah, we can do that but it is not as clear as the previous one.  The
reason is that we do similarly start/commit transaction for various
operations like autovacuum, cluster, drop index concurrently, etc.
So, it doesn't sound good to me to just change this for parallel query
and leave others as it is.

> > And then you have to decide what to do about other background
> > transactions.
>
> Not count them if they're implementation details; otherwise count them.
> For example, IMO autovacuum transactions should definitely be counted
> (as one transaction, even if they run parallel vacuum).
>

It appears to me that the definition of what we want to display in
xact_commit/xact_rollback (for pg_stat_database view) is slightly
vague.  For ex. does it mean that we will show only transactions
started by the user or does it also includes the other transactions
started internally (which you call implementation detail) to perform
the various operations?  I think users would be more interested in the
transactions initiated by them.  I think some users might also be
interested in the write transactions happened in the system,
basically, those have consumed xid.

I think we should decide what we want to do for all other internal
transactions that are started before fixing the second part of this
problem ("other transactions that parallel workers use internally").

Thanks, Robert and Alvaro to chime in as this needs some discussion to
decide what behavior we want to provide to users.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2019-Mar-23, Amit Kapila wrote:

> On Fri, Mar 22, 2019 at 10:04 AM Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > Not count them if they're implementation details; otherwise count them.
> > For example, IMO autovacuum transactions should definitely be counted
> > (as one transaction, even if they run parallel vacuum).
> 
> It appears to me that the definition of what we want to display in
> xact_commit/xact_rollback (for pg_stat_database view) is slightly
> vague.  For ex. does it mean that we will show only transactions
> started by the user or does it also includes the other transactions
> started internally (which you call implementation detail) to perform
> the various operations?  I think users would be more interested in the
> transactions initiated by them.

Yes, you're probably right.


> I think some users might also be interested in the write transactions
> happened in the system, basically, those have consumed xid.

Well, do they really want to *count* these transactions, or is it enough
to keep an eye on the "age" of some XID column?  Other than for XID
freezing purposes, I don't see such internal transactions as very
interesting.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Mar 23, 2019 at 9:50 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Mar-23, Amit Kapila wrote:
>
> > I think some users might also be interested in the write transactions
> > happened in the system, basically, those have consumed xid.
>
> Well, do they really want to *count* these transactions, or is it enough
> to keep an eye on the "age" of some XID column?  Other than for XID
> freezing purposes, I don't see such internal transactions as very
> interesting.
>

That's what I also had in mind.  I think doing anything more than just
fixing the count for the parallel cooperating transaction by parallel
workers doesn't seem intuitive to me. I mean if we want we can commit
the fix such that all supporting transactions by parallel worker
shouldn't be counted, but I am not able to convince myself that that
is the good fix.  Instead, I think rather than fixing that one case we
should think more broadly about all the supportive transactions
happening in the various operations.  Also, as that is a kind of
behavior change, we should discuss that as a separate topic.

I know what I am proposing here won't completely fix the problem Hari
is facing, but I am not sure what else we can do here which doesn't
create some form of inconsistency with other parts of the system.


--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Mar 25, 2019 at 6:55 PM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
> On Sat, Mar 23, 2019 at 11:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Sat, Mar 23, 2019 at 9:50 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> >
>> > On 2019-Mar-23, Amit Kapila wrote:
>> >
>> > > I think some users might also be interested in the write transactions
>> > > happened in the system, basically, those have consumed xid.
>> >
>> > Well, do they really want to *count* these transactions, or is it enough
>> > to keep an eye on the "age" of some XID column?  Other than for XID
>> > freezing purposes, I don't see such internal transactions as very
>> > interesting.
>> >
>>
>> That's what I also had in mind.  I think doing anything more than just
>> fixing the count for the parallel cooperating transaction by parallel
>> workers doesn't seem intuitive to me. I mean if we want we can commit
>> the fix such that all supporting transactions by parallel worker
>> shouldn't be counted, but I am not able to convince myself that that
>> is the good fix.  Instead, I think rather than fixing that one case we
>> should think more broadly about all the supportive transactions
>> happening in the various operations.  Also, as that is a kind of
>> behavior change, we should discuss that as a separate topic.
>
>
> Thanks to everyone for their opinions and suggestions to improve.
>
> Without parallel workers, there aren't many internal implementation
> logic code that causes the stats to bloat. Parallel worker stats are not
> just the transactions, it update other stats also, for eg; seqscan stats
> of a relation. I also eagree that just fixing it for transactions may not
> be a proper solution.
>
> Using of XID data may not give proper TPS of the instance as it doesn't
> involve the read only transactions information.
>
> How about adding additional two columns that provides all the internal
> and background worker transactions into that column?
>

I can see the case where the users will be interested in application
initiated transactions, so if we want to add new columns, it could be
to display that information and the current columns can keep
displaying the overall transactions in the database.   Here, I think
we need to find a way to distinguish between internal and
user-initiated transactions.  OTOH, I am not sure adding new columns
is better than changing the meaning of existing columns.  We can go
either way based on what others feel good, but I think we can do that
as a separate head-only feature.  As part of this thread, maybe we can
just fix the case of the parallel cooperating transaction.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 27, 2019 at 6:53 AM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Tue, Mar 26, 2019 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>   As part of this thread, maybe we can
>> just fix the case of the parallel cooperating transaction.
>
>
> With the current patch, all the parallel implementation transaction are getting
> skipped, in my tests parallel workers are the major factor in the transaction
> stats counter. Even before parallelism, the stats of the autovacuum and etc
> are still present but their contribution is not greatly influencing the stats.
>
> I agree with you in fixing the stats with parallel workers and improve stats.
>

I was proposing to fix only the transaction started with
StartParallelWorkerTransaction by using is_parallel_worker flag as
discussed above.  I understand that it won't completely fix the
problem reported by you, but it will be a step in that direction.  My
main worry is that if we fix it the way you are proposing and we also
invent a new way to deal with all other internal transactions, then
the fix done by us now needs to be changed/reverted.  Note, that this
fix needs to be backpatched as well, so we should avoid doing any fix
which needs to be changed or reverted.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 28, 2019 at 11:43 AM Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
> On Wed, Mar 27, 2019 at 11:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Mar 27, 2019 at 6:53 AM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>> > On Tue, Mar 26, 2019 at 9:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >>
>> >>   As part of this thread, maybe we can
>> >> just fix the case of the parallel cooperating transaction.
>> >
>> >
>> > With the current patch, all the parallel implementation transaction are getting
>> > skipped, in my tests parallel workers are the major factor in the transaction
>> > stats counter. Even before parallelism, the stats of the autovacuum and etc
>> > are still present but their contribution is not greatly influencing the stats.
>> >
>> > I agree with you in fixing the stats with parallel workers and improve stats.
>> >
>>
>> I was proposing to fix only the transaction started with
>> StartParallelWorkerTransaction by using is_parallel_worker flag as
>> discussed above.  I understand that it won't completely fix the
>> problem reported by you, but it will be a step in that direction.  My
>> main worry is that if we fix it the way you are proposing and we also
>> invent a new way to deal with all other internal transactions, then
>> the fix done by us now needs to be changed/reverted.  Note, that this
>> fix needs to be backpatched as well, so we should avoid doing any fix
>> which needs to be changed or reverted.
>
>
> I tried the approach as your suggested as by not counting the actual parallel work
> transactions by just releasing the resources without touching the counters,
> the counts are not reduced much.
>
> HEAD - With 4 parallel workers running query generates 13 stats ( 4 * 3  + 1)
> Patch -  With 4 parallel workers running query generates 9 stats ( 4 * 2 + 1)
> Old approach patch - With 4 parallel workers running query generates 1 stat (1)
>
> Currently the parallel worker start transaction 3 times in the following places.
> 1. InitPostgres
> 2. ParallelWorkerMain (2)
>
> with the attached patch, we reduce one count from ParallelWorkerMain.
>

Right, that is expected from this fix.  BTW, why you have changed the
approach in this patch to not count anything by the parallel worker as
compared to the earlier version where you were just ignoring the stats
for transactions.  As of now, either way is fine, but in future (after
parallel inserts/updates), we want other stats to be updated.  I think
your previous idea was better, just you need to start using
is_parallel_worker flag.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 3, 2019 at 10:45 AM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
> Thanks for the review.
>
> While changing the approach to use the is_parallel_worker_flag, I thought
> that the rest of the stats are also not required to be updated and also those
> are any way write operations and those values are zero anyway for parallel
> workers.
>
> Instead of expanding the patch scope, I just changed to avoid the commit
> or rollback stats as discussed, and later we can target the handling of all the
> internal transactions and their corresponding stats.
>

The patch looks good to me.  I have changed the commit message and ran
the pgindent in the attached patch.  Can you once see if that looks
fine to you?  Also, we should backpatch this till 9.6.  So, can you
once verify if the change is fine in all bank branches?   Also, test
with a force_parallel_mode option.  I have already tested it with
force_parallel_mode = 'regress' in HEAD, please test it in back
branches as well.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 8, 2019 at 7:54 AM Jamison, Kirk <k.jamison@jp.fujitsu.com> wrote:
>
> On Monday, April 8, 2019 9:04 AM (GMT+9), Haribabu Kommi wrote:
>
> >On Thu, Apr 4, 2019 at 3:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > The patch looks good to me.  I have changed the commit message and ran
> > the pgindent in the attached patch.  Can you once see if that looks
> > fine to you?  Also, we should backpatch this till 9.6.  So, can you
> > once verify if the change is fine in all bank branches?   Also, test
> > with a force_parallel_mode option.  I have already tested it with
> > force_parallel_mode = 'regress' in HEAD, please test it in back
> > branches as well.
> >
> > Thanks for the updated patch.
> > I tested in back branches even with force_parallelmode and it is working
> > as expected. But the patches apply is failing in back branches, so attached
> > the patches for their branches. For v11 it applies with hunks.
>
> There are 3 patches for this thread:
> _v5: for PG v11 to current head
> _10: for PG10 branch
> _96: for PG9.6
>
> I have also tried applying these latest patches, .
> The patch set works correctly from patch application, build to compilation.
> I also tested with force_parallel_mode, and it works as intended.
>
> So I am marking this thread as “Ready for Committer”.
>

Thanks, Hari and Jamison for verification.  The patches for
back-branches looks good to me.  I will once again verify them before
commit. I will commit this patch tomorrow unless someone has
objections.  Robert/Alvaro, do let me know if you see any problem with
this fix?

> I hope this makes it on v12 before the feature freeze.
>

Yes,  I think fixing bugs should be fine unless we delay too much.

I see one typo in the commit message (transactions as we that is what
we do in ../transactions as that is what we do in ..), will fix it
before commit.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 8, 2019 at 8:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 8, 2019 at 7:54 AM Jamison, Kirk <k.jamison@jp.fujitsu.com> wrote:
> > So I am marking this thread as “Ready for Committer”.
> >
>
> Thanks, Hari and Jamison for verification.  The patches for
> back-branches looks good to me.  I will once again verify them before
> commit. I will commit this patch tomorrow unless someone has
> objections.  Robert/Alvaro, do let me know if you see any problem with
> this fix?
>

Pushed.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com