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.
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.
Regards,
Haribabu Kommi
Fujitsu Australia