Subject: Re: snapbuild woes
To: Andres Freund <andres@anarazel.de>
References: <51f65289-54f8-2256-d107-937d662d69f1@2ndquadrant.com>
 <f41d64c7-1ae2-fac7-bf7f-f24dfe2d33bd@2ndquadrant.com>
 <20170413002959.apytyi5lkknauauj@alap3.anarazel.de>
 <0d06dfc1-0bc5-c3ab-3555-24e2f275d7fa@2ndquadrant.com>
 <30242bc6-eca4-b7bb-670e-8d0458753a8c@2ndquadrant.com>
 <20170501005921.qcqd7w2njpv3av6s@alap3.anarazel.de>
 <20170501013529.sejtgo2acxbvq26q@alap3.anarazel.de>
 <209861fc-718f-0a7b-3a1d-36512865c209@2ndquadrant.com>
 <20170501080348.vb6mj5whdex3f7l7@alap3.anarazel.de>
 <e6c1fdfe-50bc-62e4-3ad7-854f6ac5c35e@2ndquadrant.com>
 <20170501191419.coqbtx2jodu22ufz@alap3.anarazel.de>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Erik Rijkers <er@xs4all.nl>, Robert Haas <robertmhaas@gmail.com>,
 Craig Ringer <craig@2ndquadrant.com>
From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Message-ID: <ba7a3052-6e71-d162-d4a0-b5bdd7145c19@2ndquadrant.com>
Date: Tue, 2 May 2017 08:55:53 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170501191419.coqbtx2jodu22ufz@alap3.anarazel.de>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

On 01/05/17 21:14, Andres Freund wrote:
> On 2017-05-01 11:09:44 +0200, Petr Jelinek wrote:
>> On 01/05/17 10:03, Andres Freund wrote:
>>> On 2017-05-01 03:54:49 +0200, Petr Jelinek wrote:
> 
>>>> But, I still think we need to restart the tracking after new
>>>> xl_running_xacts. Reason for that is afaics any of the catalog snapshots
>>>> that we assigned to transactions at the end of SnapBuildCommitTxn might
>>>> be corrupted otherwise as they were built before we knew one of the
>>>> supposedly running txes was actually already committed and that
>>>> transaction might have done catalog changes.
>>>
>>> I'm afraid you're right.  But I think this is even more complicated: The
>>> argument in your version that this can only happen once, seems to also
>>> be holey: Just imagine a pg_usleep(3000 * 1000000) right before
>>> ProcArrayEndTransaction() and enjoy the picture.
> 
>> Well yes, transaction can in theory have written commit/abort xlog
>> record and stayed in proc for more than single xl_running_xacts write.
>> But then the condition which we test that the new xl_running_xacts has
>> bigger xmin than the previously tracked one's xmax would not be
>> satisfied and we would not enter the relevant code path yet. So I think
>> we should not be able to get any xids we didn't see. But we have to
>> restart tracking from beginning (after first checking if we didn't
>> already see anything that the xl_running_xacts considers as running),
>> that's what my code did.
> 
> But to get that correct, we'd have to not only track ->committed, but
> also somehow maintain ->aborted, and not just for the transactions in
> the original set of running transactions.  That'd be fairly complicated
> and large.  The reason I was trying - and it's definitely not correct as
> I had proposed - to use the original running_xacts record is that that
> only required tracking as many transaction statuses as in the first
> xl_running_xacts.  Am I missing something?

Aah, now I understand we talked about slightly different things, I
considered the running thing to be first step towards tracking aborted
txes everywhere. I am not sure if it's complicated, it would be exactly
the same as committed tracking, except we'd do it only before we reach
SNAPBUILD_CONSISTENT. It would be definitely larger patch I agree, but I
can give it at try.

If you think that adding the SNAPBUILD_BUILD_INITIAL_SNAPSHOT would be
less invasive/smaller patch I am okay with doing that for PG10. I think
we'll have to revisit tracking of aborted transactions in PG11 then
though because of the 'snapshot too large' issue when exporting, at
least I don't see any other way to fix that.

> 
> The probabilistic tests catch the issues here fairly quickly, btw, if
> you run with synchronous_commit=on, while pgbench is running, because
> the WAL flushes make this more likely.  Runs this query:
> 
> SELECT account_count, teller_count, account_sum - teller_sum s
> FROM
>     (
>         SELECT count(*) account_count, SUM(abalance) account_sum
>         FROM pgbench_accounts
>     ) a,
>     (
>         SELECT count(*) teller_count, SUM(tbalance) teller_sum
>         FROM pgbench_tellers
>     ) t
> 
> which, for my scale, should always return:
> ┌─────────┬─────┬───┐
> │    a    │  t  │ s │
> ├─────────┼─────┼───┤
> │ 2000000 │ 200 │ 0 │
> └─────────┴─────┴───┘
> but with my POC patch occasionally returns things like:
> ┌─────────┬─────┬───────┐
> │    a    │  t  │   s   │
> ├─────────┼─────┼───────┤
> │ 2000000 │ 212 │ 37358 │
> └─────────┴─────┴───────┘
> 
> which obviously shouldn't be the case.
> 

Very nice (the test, not the failures ;)) !

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers