Subject: Re: standby recovery fails (tablespace related) (tentative patch and discussion)

From: Alvaro Herrera <[email-masked]>
Date: Mon, 21 Mar 2022 20:03:07 +0100

On 2022-Mar-14, Robert Haas wrote:

> 2. Why not instead change the code so that the operation can succeed,
> by creating the prerequisite parent directories? Do we not have enough
> information for that? I'm not saying that we definitely should do it
> that way rather than this way, but I think we do take that approach in
> some cases.

It seems we can choose freely between these two implementations -- I
mean I don't see any upsides or downsides to either one.

The current one has the advantage that it never makes the datadir
"dirty", to use Kyotaro's term.  It verifies that the creation/drop form
a pair.  A possible downside is that if there's a bug, we could end up
with a spurious PANIC at the end of recovery, and no way to recover.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

---

From: Robert Haas <[email-masked]>
Date: Mon, 28 Mar 2022 12:17:50 -0400

> mean I don't see any upsides or downsides to either one.

What got committed here feels inconsistent to me. Suppose we have a
checkpoint, and then a series of operations that touch a tablespace,
and then a drop database and drop tablespace. If the first operation
happens to be CREATE DATABASE, then this patch is going to fix it by
skipping the operation. However, if the first operation happens to be
almost anything else, the way it's going to reference the dropped
tablespace is via a block reference in a WAL record of a wide variety
of types. That's going to result in a call to
XLogReadBufferForRedoExtended() which will call
XLogReadBufferExtended() which will do smgrcreate(smgr, forknum, true)
which will in turn call TablespaceCreateDbspace() to fill in all the
missing directories.

I don't think that's very good. It would be reasonable to decide that
we're never going to create the missing directories and instead just
remember that they were not found so we can do a cross check. It's
also reasonable to just create the directories on the fly. But doing a
mix of those systems doesn't really seem like the right idea -
particularly because it means that the cross-check system is probably
not very effective at finding actual problems in the code.

Am I missing something here?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 29 Mar 2022 13:55:35 +0900 (JST)

At Mon, 28 Mar 2022 12:17:50 -0400, Robert Haas <[email-masked]> wrote in 
> which will in turn call TablespaceCreateDbspace() to fill in all the

Right. I thought that recovery avoids that but that's wrong.  This
behavior creates a bare (non-linked) directly within pg_tblspc.  The
directory would dissapear soon if recovery proceeds to the consistency
point, though.

> Am I missing something here?

No. I agree that mixing them is not good.  On the other hand we
already doing that by heapam.  AFAICS sometimes it avoid creating a
new page but sometimes creates it.  But I don't mean to use the fact
for justifying this patch to do that, or denying to do that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 29 Mar 2022 13:37:34 +0200

On 2022-Mar-29, Kyotaro Horiguchi wrote:

> > That's going to result in a call to

Hmm, this is not good.

> for justifying this patch to do that, or denying to do that.

I think we should revert this patch and do it again using the other
approach: create a stub directory during recovery that can be deleted
later.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

---

From: Robert Haas <[email-masked]>
Date: Tue, 29 Mar 2022 08:45:13 -0400

> approach: create a stub directory during recovery that can be deleted

I'm fine with that approach, but I'd like to ask that we proceed
expeditiously, because I have another patch that I want to commit that
touches this area. I can commit to helping with whatever we decide to
do here, but I don't want to keep that patch on ice while we figure it
out and then have it miss the release.

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 29 Mar 2022 15:28:26 +0200

> out and then have it miss the release.

OK, this is a bug that's been open for years.   A fix can be committed
after the feature freeze anyway.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

---

From: Robert Haas <[email-masked]>
Date: Tue, 29 Mar 2022 09:31:42 -0400

> after the feature freeze anyway.

+1