Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

On 2014-07-26 20:20:05 +0200, Andres Freund wrote:
> On 2014-07-26 13:58:38 -0400, Tom Lane wrote:
> 
> > Andres Freund  writes:
> > > That'd require either renegging on SA_RESTART or
> > > using WaitLatchOrSocket() and nonblocking send/recv.
> > 
> > Yeah, I was wondering about using WaitLatchOrSocket for client I/O too.
> > We already have a hook that lets us do the actual recv even when using
> > OpenSSL, and in principle that function could do interrupt-service-like
> > functions if it got kicked off the recv().
> 
> I've started playing with this. Looks clearly worthwile.
> 
> I think if we do it right we pretty much can get rid of the whole
> prepare_for_client_read() machinery and handle everything via
> ProcessInterrupts(). EnableCatchupInterrupt() et al don't really fill me
> with joy.
> 
> I'm not yet entirely sure where the interrupt processing should happen,
> but I guess that'll fall out of the work at some point. The important
> bit imo is to *not* not do anything but return with BIO_set_retry_*()
> from my_sock_read/write(). That then allows us to implement stuff like
> the idle transaction timeout with much fewer problems.
> 
> I probably won't finish doing this before leaving on holidays, so nobody
> should hesitate to look themselves if interested. If not, I plan to pick
> this up again.  I think it's a prerequisite to getting rid of the FATAL
> for recovery conflict interrupts which I really would like to do.

I tried to get something reviewable before leaving, but alas, there's
far too many edgecases to consider. A good part of it has to do with my
decision to always operate the underlying socket in nonblocking mode and
do all the writing using latches. I think that's the right decision,
because it allows reliable interruptions everywhere and is easier than
duplicated codepaths. But I realize that's not guaranteed to be well
liked.

a) Latches aren't ready early enough. We need to read/write to the
socket long before MyProc is initialized. To have sane behaviour during
early startup we need to be canceleable there was well.

Right now I'm just busy looping in that case, but that's obviously not
acceptable. My best idea is to have another latch that we can use during
early startup. Say *MyProcLatch. Initially that points to a process
local latch and once initialized it points to MyProc->procLatch.

b) Latches don't support WL_WRITEABLE without WL_READABLE. I've simply
changed the code in both latch implementations to poll for errors in
both cases and set both readable/writeable if an error occurs. That
seems easy enough. Are there any bigger caveats for changing this?

c) There's a couple more or less undocumented
pgwin32_waitforsinglesocket() calls in be-secure.c. Afaics they're
likely partially already not required and definitely not required after
socket handling is baded on latches.

d) prepare_for_client_read(), client_read_ended() are now really quite a
misnomer. Because interrupts interrupt send/recv appropriately and
because we do *not* want to process them inside my_sock_read|write (so
we don't recursively enter openssl to send the FATAL to the client)
they're not used from within ssl anymore. But on a higher level, just
like:
    prepare_for_client_read();
    CHECK_FOR_INTERRUPTS();
    client_read_ended();
Not sure if that's the right abstraction for just a:
    EnableNotifyInterrupt();
    EnableCatchupInterrupt();
    CHECK_FOR_INTERRUPTS();
    DisableNotifyInterrupt();
    DisableCatchupInterrupt();
where the Enable* just set a variable so CHECK_FOR_INTERRUPTS can
process the events if occuring.

Doing it that way allows to throw away *large* chunks of code from
sinval.c and async.c because they don't have to care about doing
dangerous stuff from signal handlers anymore.

But the above needs a new name.


Even though it ends up being a bit more work than I anticipated, the
result still seems like a significant improvement over the current
code.

Will get back to this once I'm back (~10 days).  Unless somebody else
wants to pick this up. I've at attached my *heavily WIP* patch.

Greetings,

Andres Freund

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

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

[RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

I've tracked down the real root cause.  The fix is very simple.  Please 
check the attached one-liner patch.

The cause is that the temporary relations are truncated unconditionally 
regardless of whether they are accessed in the transaction or not.  That is, 
the following sequence of steps result in the hang:

1. A session creates a temporary table with ON COMMIT DELETE ROWS.  It adds 
the temporary table to the list of relations that should be truncated at 
transaction commit.

2. The session receives a sinval catchup signal (SIGUSR1) from another 
session.  It starts a transaction and processes sinval messages in the 
SIGUSR1 signal handler.  No WAL is output while processing the sinval 
messages.

3. When the transaction commits, the list of temporary relations are checked 
to see if they need to be truncated.

4. The temporary table created in step 1 is truncated.  To truncate a 
relation, Access Exclusive lock is acquired on it.  When hot standby is 
used, acquiring an Access Exclusive lock generates a WAL record 
(RM_STANDBY_ID, XLOG_STANDBY_LOCK).

5. The transaction waits on a latch for a reply from a synchronous standby, 
because it wrote some WAL.  But the latch wait never returns, because the 
latch needs to receive SIGUSR1 but the SIGUSR1 handler is already in 
progress from step 2.


The correct behavior is for the transaction not to truncate the temporary 
table in step 4, because the transaction didn't use the temporary table.

I confirmed that the fix is already in 9.3 and 9.5devel, so I just copied 
the code fragment from 9.5devel to 9.2.9.  The attached patch is for 9.2.9. 
I didn't check 9.4 and other versions.  Why wasn't the fix applied to 9.2?

Finally, I found a very easy way to reproduce the problem:

1. On terminal session 1, start psql and run:
  CREATE TEMPORARY TABLE t (c int) ON COMMIT DELETE ROWS;
Leave the psql session open.

2. On terminal session 2, run:
  pgbench -c8 -t500 -s1 -n -f test.sql dbname
[test.sql]
CREATE TEMPORARY TABLE t (c int) ON COMMIT DELETE ROWS;
DROP TABLE t;

3. On the psql session on terminal session 1, run any SQL statement.  It 
doesn't reply.  The backend is stuck at SyncRepWaitForLSN().

Regards
MauMau

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Please find attached the revised patch.  I failed to change SIGUSR1 to 
SIGUSR2 when sending sinval catchup signal.

In addition, I changed wal sender to not receive SIGUSR1/2 from 
SendProcSignal().  Without this, wal sender will excessively wake up or 
terminate by the signals.  This is an existing problem.

Regards
MauMau

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

Re: [RFC] Should smgrtruncate() avoid sending sinval message for temp relations

From: "MauMau" 
>> I'll try the fix tomorrow if possible.  What kind of problems do you hink 
>> of for back-patching?
>
> I could reproduce the problem with 9.2.8, but have not yet with 9.5dev. 
> I'll try with 9.2.9, and create the fix.

I could also reproduce the problem with 9.2.9, but I couldn't with 9.5devel.

However, I could confirm that the attached patch solves the problem.  The 
patch is based on 9.2.9.  To adjust this patch for 9.3 and later, set the 
background worker's SIGUSR2 handler to procsignal_sigusr2_handler like 
normal backends.  Could you review and commit this?  We wish the fix for 9.1 
and above.

To reproduce the problem, you can do as follows with the attached files. 
Originally, test.sql had many columns the customer is actually using, but we 
cannot show the real DDL as it's the customer's asset.

$ createdb test
$ pgbench -c 20 -j 20 -T 600 -s -n -f test.pgbench

Synchronous streaming replication and hot standby need to be used.

When I ran this on a 4-core RHEL5 box, the problem arose within a few 
minutes.  pgbench continues to run emitting a lot of messages like below. 
Over time, the number of normal backends will increases, and dozens of which 
remain stuck with the stack trace I showed before.

CREATE TABLE
psql:test.sql:1: NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit 
index "test_table_pkey" for table "test_table"

I have no idea why 9.5devel didn't show the problem.  One difference I 
noticed is that pgbench didn't output the message of implicit index creation 
for the primary key.

Regards
MauMau