Обсуждение: [PROPOSAL] Drop orphan temp tables in single-mode

Hello hackers,

In some cases if PostgreSQL encounters with wraparound PostgreSQL might 
leave created temporary tables even after shutdown.

This orphan temporary tables prevent VACUUM to fix wraparound. It is 
because in single mode VACUUM considers orphan temp tables as temp 
tables of other backends.

Grigory reported that one of our client did stuck with fixing wraparound 
by because he didn't know that he has orphaned temp tables left by a 
backend after wraparound.

This patch fixes the issue. With it VACUUM deletes orphaned tables in 
single mode.

See also thread in general (I'm not sure that orphan temp tables were 
cause here though):
https://www.postgresql.org/message-id/CADU5SwN6u4radqQgUY2VjEyqXF0KJ6A09PYuJjT%3Do9d7vzM%3DCg%40mail.gmail.com

If the patch is interesting I'll add it to the next commitfest and label 
it as 'v13'.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hi!

On Thu, Mar 7, 2019 at 12:46 PM Arthur Zakirov <a.zakirov@postgrespro.ru> wrote:
> In some cases if PostgreSQL encounters with wraparound PostgreSQL might
> leave created temporary tables even after shutdown.
>
> This orphan temporary tables prevent VACUUM to fix wraparound. It is
> because in single mode VACUUM considers orphan temp tables as temp
> tables of other backends.
>
> Grigory reported that one of our client did stuck with fixing wraparound
> by because he didn't know that he has orphaned temp tables left by a
> backend after wraparound.
>
> This patch fixes the issue. With it VACUUM deletes orphaned tables in
> single mode.
>
> See also thread in general (I'm not sure that orphan temp tables were
> cause here though):
> https://www.postgresql.org/message-id/CADU5SwN6u4radqQgUY2VjEyqXF0KJ6A09PYuJjT%3Do9d7vzM%3DCg%40mail.gmail.com
>
> If the patch is interesting I'll add it to the next commitfest and label
> it as 'v13'.

As far as I understand, it's intended that user should be able to fix
wraparound in single mode.  Assuming this issue may prevent user form
doing this and fix is quite trivial, should we consider backpatching
this?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Arthur Zakirov <a.zakirov@postgrespro.ru> writes:
> In some cases if PostgreSQL encounters with wraparound PostgreSQL might 
> leave created temporary tables even after shutdown.
> This orphan temporary tables prevent VACUUM to fix wraparound. It is 
> because in single mode VACUUM considers orphan temp tables as temp 
> tables of other backends.

Hm.

> This patch fixes the issue. With it VACUUM deletes orphaned tables in 
> single mode.

This seems like an astonishingly bad idea.  Nobody would expect DROP TABLE
to be spelled "VACUUM", and the last thing we need when someone has been
forced to use single-user mode is to put additional land mines under their
feet.  They might, for example, wish to do forensic investigation on such
tables to discover the reason for a crash.

I wonder if a better response would be, in single-user mode, to allow temp
tables to be processed as local temp tables regardless of their backend
number.  (Everywhere, not just in VACUUM.)

Also, if what someone actually wants is to drop such a temp table from
single-user mode, we should make sure that they are allowed to do so.
But the command for doing that should be "DROP TABLE", not "VACUUM".

            regards, tom lane

On Thu, Mar 7, 2019 at 9:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wonder if a better response would be, in single-user mode, to allow temp
> tables to be processed as local temp tables regardless of their backend
> number.  (Everywhere, not just in VACUUM.)

Since commit debcec7dc31a992703911a9953e299c8d730c778 there is nothing
to prevent two different backends from using the same relfilenode
number, so I don't think this will work.

> Also, if what someone actually wants is to drop such a temp table from
> single-user mode, we should make sure that they are allowed to do so.
> But the command for doing that should be "DROP TABLE", not "VACUUM".

In a way I agree, but I think the reality is that some very large
percentage of people who enter single user mode do so because of a
wraparound-induced shutdown, and what they need is an easy way to get
the system back on line.  Running a catalog query to look for
undropped temp tables and then dropping them one by one using DROP
TABLE is not what they want.  They want to be able to run one or two
commands and get their database back on line.

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

Robert Haas <robertmhaas@gmail.com> writes:
> In a way I agree, but I think the reality is that some very large
> percentage of people who enter single user mode do so because of a
> wraparound-induced shutdown, and what they need is an easy way to get
> the system back on line.  Running a catalog query to look for
> undropped temp tables and then dropping them one by one using DROP
> TABLE is not what they want.  They want to be able to run one or two
> commands and get their database back on line.

So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
let's do that.  But please let's not turn a well defined command
like VACUUM into something that you don't quite know what it will do.
I'm especially down on having squishy DWIM logic in a last-resort
operating mode; the fact that some people only use it in a certain way
is a poor excuse for setting booby traps for everybody.

Something I could get behind as a less dangerous way of addressing
this issue is to define DISCARD TEMP, in single-user mode, as dropping
the contents of all temp schemas not just one.

            regards, tom lane

On Thu, Mar 7, 2019 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
> let's do that.  But please let's not turn a well defined command
> like VACUUM into something that you don't quite know what it will do.

I am on the fence about that.  I see your point, but on the other
hand, autovacuum drops temp tables all the time in multi-user mode and
I think it's pretty clear that, with the possible exception of you,
users find that an improvement.  So it could be argued that we're
merely proposing to make the single-user mode behavior of vacuum
consistent with the behavior people are already expecting it to do.

The underlying and slightly more general problem here is that users
find it really hard to know what to do when vacuum fails to advance
relfrozenxid.  Of course, temp tables are only one reason why that can
happen: logical decoding slots and prepared transactions are others,
and I don't think we can automatically drop that stuff because
somebody may still be expecting them to accomplish whatever their
intended purpose is.  The difference with temp tables is that users
imagine -- quite naturally I think -- that they are in fact temporary,
and that they will in fact go away when the session ends.  The user
would tend to view their continued existence as an unwanted
implementation artifact, not something that they should be responsible
for removing.

Really, I'd like to redesign the way this whole system works.  Instead
of forcing a full-system shutdown, we should just refuse to assign any
more XIDs, disable the vacuum cost delay machinery, and let autovacuum
go nuts until the problem is corrected.  Forcing people to run vacuum
to run one vacuum at a time is not nice, and not having background
processes like the bgwriter or checkpointer while you're doing it
isn't good either, and there's no good reason to disallow SELECT
queries while we're recovering the system either. Actually, even
before we get to the point where we currently force a shutdown, we
ought to just give up on vacuum cost delay, either all at once or
perhaps incrementally, when we see that we're getting into trouble.
But all of that is work for another time.

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

> On Thu, Mar 7, 2019 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
> > let's do that.  But please let's not turn a well defined command
> > like VACUUM into something that you don't quite know what it will do.

I see your point. Another approach would be to let user know what prevents
VACUUM to fix the wraparound in single mode, mainly that there are orphan
temp tables . But it might be too verbose if PostgreSQL will print warning for
every orphan temp table.

> Really, I'd like to redesign the way this whole system works.  Instead
> of forcing a full-system shutdown, we should just refuse to assign any
> more XIDs, disable the vacuum cost delay machinery, and let autovacuum
> go nuts until the problem is corrected.  Forcing people to run vacuum
> to run one vacuum at a time is not nice, and not having background
> processes like the bgwriter or checkpointer while you're doing it
> isn't good either, and there's no good reason to disallow SELECT
> queries while we're recovering the system either. Actually, even
> before we get to the point where we currently force a shutdown, we
> ought to just give up on vacuum cost delay, either all at once or
> perhaps incrementally, when we see that we're getting into trouble.
> But all of that is work for another time.

I think it would be very neat feature!

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On 03/07/2019 06:49 PM, Robert Haas wrote:
> On Thu, Mar 7, 2019 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
>> let's do that.  But please let's not turn a well defined command
>> like VACUUM into something that you don't quite know what it will do.
> I am on the fence about that.  I see your point, but on the other
> hand, autovacuum drops temp tables all the time in multi-user mode and
> I think it's pretty clear that, with the possible exception of you,
> users find that an improvement.  So it could be argued that we're
> merely proposing to make the single-user mode behavior of vacuum
> consistent with the behavior people are already expecting it to do.
>
> The underlying and slightly more general problem here is that users
> find it really hard to know what to do when vacuum fails to advance
> relfrozenxid.  Of course, temp tables are only one reason why that can
> happen: logical decoding slots and prepared transactions are others,
> and I don't think we can automatically drop that stuff because
> somebody may still be expecting them to accomplish whatever their
> intended purpose is.  The difference with temp tables is that users
> imagine -- quite naturally I think -- that they are in fact temporary,
> and that they will in fact go away when the session ends.  The user
> would tend to view their continued existence as an unwanted
> implementation artifact, not something that they should be responsible
> for removing.

I`m no hacker, but I would like to express my humble opinion on the matter.
1. Proposed patch is fairly conservative, to be on fully consistent with 
autovacuum behaivour VACUUM should be able to drop orphaned temp table 
even in mult-user mode.

2. There is indeed a problem of expected behavior from user perspective. 
Every PostgreSQL user knows that if you hit wraparound, you go 
single-mode, run VACUUM and the problem goes away. Exactly because of 
this I`ve got involved with this problem:
https://www.postgresql.org/message-id/0c7c2f84-74f5-2cd9-767e-9b2566065d71%40postgrespro.ru
Poor guy repeatedly run VACUUM after VACUUM and had no clue what to do. 
He even considered to just restore from backup and be done with it. It 
took some time to figure out a true culprit, and time = money.

-- 
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Mar 07, 2019 at 10:49:29AM -0500, Robert Haas wrote:
> On Thu, Mar 7, 2019 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
> > let's do that.  But please let's not turn a well defined command
> > like VACUUM into something that you don't quite know what it will do.
>
> I am on the fence about that.  I see your point, but on the other
> hand, autovacuum drops temp tables all the time in multi-user mode and
> I think it's pretty clear that, with the possible exception of you,
> users find that an improvement.  So it could be argued that we're
> merely proposing to make the single-user mode behavior of vacuum
> consistent with the behavior people are already expecting it to do.

It is possible for a session to drop temporary tables of other
sessions.  Wouldn't it work as well in this case for single-user mode
when seeing an orphan temp table still defined?  Like Tom, I don't
think that it is a good idea to play with the heuristics of VACUUM in
the way the patch proposes.
--
Michael

On Fri, Mar 8, 2019 at 9:28 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Mar 07, 2019 at 10:49:29AM -0500, Robert Haas wrote:
> > On Thu, Mar 7, 2019 at 10:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > So if we think we can invent a "MAGICALLY FIX MY DATABASE" command,
> > > let's do that.  But please let's not turn a well defined command
> > > like VACUUM into something that you don't quite know what it will do.
> >
> > I am on the fence about that.  I see your point, but on the other
> > hand, autovacuum drops temp tables all the time in multi-user mode and
> > I think it's pretty clear that, with the possible exception of you,
> > users find that an improvement.  So it could be argued that we're
> > merely proposing to make the single-user mode behavior of vacuum
> > consistent with the behavior people are already expecting it to do.
>
> It is possible for a session to drop temporary tables of other
> sessions.  Wouldn't it work as well in this case for single-user mode
> when seeing an orphan temp table still defined?  Like Tom, I don't
> think that it is a good idea to play with the heuristics of VACUUM in
> the way the patch proposes.

I think we have a kind of agreement, that having simple way to get rid
of all orphan tables in single-user mode is good.  The question is
whether it's good for VACUUM command to do this.  Naturally, user may
enter single-user mode for different reasons, not only for xid
wraparound fixing.  For example, one may enter this mode for examining
temporary tables present in the database.  Then it would be
disappointing surprise that all of them gone after VACUUM command.

So, what about special option, which would make VACUUM to drop orphan
tables in single-user mode?  Do we need it in multi-user mode too?

BTW, does this patch checks that temporary table is really orphan?
AFAICS, user may define some temporary tables in single-user mode
before running VACUUM.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello Alexander,

On Friday, June 7, 2019, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
> BTW, does this patch checks that temporary table is really orphan?
> AFAICS, user may define some temporary tables in single-user mode
> before running VACUUM.

As far as I remember, the patch checks it.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Artur