Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_dump: Refactor getIndexes()

  2. Optimize DropRelFileNodesAllBuffers() for recovery.

  1. In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-11T08:33:17Z

    Hello. This is a thread for an alternative solution to wal_level=none
[*1] for bulk data loading.

*1: https://www.postgresql.org/message-id/TYAPR01MB29901EBE5A3ACCE55BA99186FE320%40TYAPR01MB2990.jpnprd01.prod.outlook.com

At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <sfrost@snowman.net> wrote in 
> Greetings,
> 
> * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > trials of several ways, I drifted to the following way after poking
> > several ways.
> > 
> > 1. Flip BM_PERMANENT of active buffers
> > 2. adding/removing init fork
> > 3. sync files,
> > 4. Flip pg_class.relpersistence.
> > 
> > It always skips table copy in the SET UNLOGGED case, and only when
> > wal_level=minimal in the SET LOGGED case.  Crash recovery seems
> > working by some brief testing by hand.
> 
> Somehow missed that this patch more-or-less does what I was referring to
> down-thread, but I did want to mention that it looks like it's missing a
> necessary FlushRelationBuffers() call before the sync, otherwise there
> could be dirty buffers for the relation that's being set to LOGGED (with
> wal_level=minimal), which wouldn't be good.  See the comments above
> smgrimmedsync().

Right. Thanks.  However, since SetRelFileNodeBuffersPersistence()
called just above scans shared buffers so I don't want to just call
FlushRelationBuffers() separately.  Instead, I added buffer-flush to
SetRelFileNodeBuffersPersistence().

FWIW this is a revised version of the PoC, which has some known
problems.

- Flipping of Buffer persistence is not WAL-logged nor even be able to
  be safely roll-backed. (It might be better to drop buffers).

- This version handles indexes but not yet handle toast relatins.

- tableAMs are supposed to support this feature. (but I'm not sure
  it's worth allowing them not to do so).

> > Of course, I haven't performed intensive test on it.
> 
> Reading through the thread, it didn't seem very clear, but we should
> definitely make sure that it does the right thing on replicas when going
> between unlogged and logged (and between logged and unlogged too), of
> course.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: In-place persistance change of a relation

    Stephen Frost <sfrost@snowman.net> — 2020-11-11T14:56:44Z

    Greetings,

* Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <sfrost@snowman.net> wrote in 
> > * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > > trials of several ways, I drifted to the following way after poking
> > > several ways.
> > > 
> > > 1. Flip BM_PERMANENT of active buffers
> > > 2. adding/removing init fork
> > > 3. sync files,
> > > 4. Flip pg_class.relpersistence.
> > > 
> > > It always skips table copy in the SET UNLOGGED case, and only when
> > > wal_level=minimal in the SET LOGGED case.  Crash recovery seems
> > > working by some brief testing by hand.
> > 
> > Somehow missed that this patch more-or-less does what I was referring to
> > down-thread, but I did want to mention that it looks like it's missing a
> > necessary FlushRelationBuffers() call before the sync, otherwise there
> > could be dirty buffers for the relation that's being set to LOGGED (with
> > wal_level=minimal), which wouldn't be good.  See the comments above
> > smgrimmedsync().
> 
> Right. Thanks.  However, since SetRelFileNodeBuffersPersistence()
> called just above scans shared buffers so I don't want to just call
> FlushRelationBuffers() separately.  Instead, I added buffer-flush to
> SetRelFileNodeBuffersPersistence().

Maybe I'm missing something, but it sure looks like in the patch that
SetRelFileNodeBuffersPersistence() is being called after the
smgrimmedsync() call, and I don't think you get to just switch the order
of those- the sync is telling the kernel to make sure it's written to
disk, while the FlushBuffer() is just writing it into the kernel but
doesn't provide any guarantee that the data has actually made it to
disk.  We have to FlushBuffer() first, and then call smgrimmedsync().
Perhaps there's a way to avoid having to go through shared buffers
twice, and I generally agreed it'd be good if we could avoid doing so,
but this approach doesn't look like it actually works.

> FWIW this is a revised version of the PoC, which has some known
> problems.
> 
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
>   be safely roll-backed. (It might be better to drop buffers).

Not sure if it'd be better to drop buffers or not, but figuring out how
to deal with rollback seems pretty important.  How is the persistence
change in the catalog not WAL-logged though..?

> - This version handles indexes but not yet handle toast relatins.

Would need to be fixed, of course.

> - tableAMs are supposed to support this feature. (but I'm not sure
>   it's worth allowing them not to do so).

Seems like they should.

Thanks,

Stephen

  3. Re: In-placre persistance change of a relation

    Andres Freund <andres@anarazel.de> — 2020-11-11T22:18:04Z

    Hi,

I suggest outlining what you are trying to achieve here. Starting a new
thread and expecting people to dig through another thread to infer what
you are actually trying to achive isn't great.

FWIW, I'm *extremely* doubtful it's worth adding features that depend on
a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
I understand.  If somebody added support for dynamically adapting
wal_level (e.g. wal_level=auto, that increases wal_level to
replica/logical depending on the presence of replication slots), it'd
perhaps be different.


On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> FWIW this is a revised version of the PoC, which has some known
> problems.
> 
> - Flipping of Buffer persistence is not WAL-logged nor even be able to
>   be safely roll-backed. (It might be better to drop buffers).

That's obviously a no-go. I think you might be able to address this if
you accept that the command cannot be run in a transaction (like
CONCURRENTLY). Then you can first do the catalog changes, change the
persistence level, and commit.

Greetings,

Andres Freund

  4. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-12T06:55:24Z

    At Wed, 11 Nov 2020 14:18:04 -0800, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> I suggest outlining what you are trying to achieve here. Starting a new
> thread and expecting people to dig through another thread to infer what
> you are actually trying to achive isn't great.

Agreed. I'll post that. Thanks.

> FWIW, I'm *extremely* doubtful it's worth adding features that depend on
> a PGC_POSTMASTER wal_level=minimal being used. Which this does, a far as
> I understand.  If somebody added support for dynamically adapting
> wal_level (e.g. wal_level=auto, that increases wal_level to
> replica/logical depending on the presence of replication slots), it'd
> perhaps be different.

Yes, this depends on wal_level=minimal for switching from UNLOGGED to
LOGGED, that's similar to COPY/INSERT-to-intransaction-created-tables
optimization for wal_level=minimal.  And it expands that optimization
to COPY/INSERT-to-existent-tables, which seems worth doing.

Switching to LOGGED needs to emit the initial state to WAL... Hmm.. I
came to think that even in that case skipping table copy reduces I/O
significantly, even though FPI-WAL is emitted.


> On 2020-11-11 17:33:17 +0900, Kyotaro Horiguchi wrote:
> > FWIW this is a revised version of the PoC, which has some known
> > problems.
> > 
> > - Flipping of Buffer persistence is not WAL-logged nor even be able to
> >   be safely roll-backed. (It might be better to drop buffers).
> 
> That's obviously a no-go. I think you might be able to address this if
> you accept that the command cannot be run in a transaction (like
> CONCURRENTLY). Then you can first do the catalog changes, change the
> persistence level, and commit.

Of course.  The next version reverts persistence change at abort.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: In-place persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-12T06:55:37Z

    At Wed, 11 Nov 2020 09:56:44 -0500, Stephen Frost <sfrost@snowman.net> wrote in 
> Greetings,
> 
> * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > At Tue, 10 Nov 2020 09:33:12 -0500, Stephen Frost <sfrost@snowman.net> wrote in 
> > > * Kyotaro Horiguchi (horikyota.ntt@gmail.com) wrote:
> > > > For fuel(?) of the discussion, I tried a very-quick PoC for in-place
> > > > ALTER TABLE SET LOGGED/UNLOGGED and resulted as attached. After some
> > > > trials of several ways, I drifted to the following way after poking
> > > > several ways.
> > > > 
> > > > 1. Flip BM_PERMANENT of active buffers
> > > > 2. adding/removing init fork
> > > > 3. sync files,
> > > > 4. Flip pg_class.relpersistence.
> > > > 
> > > > It always skips table copy in the SET UNLOGGED case, and only when
> > > > wal_level=minimal in the SET LOGGED case.  Crash recovery seems
> > > > working by some brief testing by hand.
> > > 
> > > Somehow missed that this patch more-or-less does what I was referring to
> > > down-thread, but I did want to mention that it looks like it's missing a
> > > necessary FlushRelationBuffers() call before the sync, otherwise there
> > > could be dirty buffers for the relation that's being set to LOGGED (with
> > > wal_level=minimal), which wouldn't be good.  See the comments above
> > > smgrimmedsync().
> > 
> > Right. Thanks.  However, since SetRelFileNodeBuffersPersistence()
> > called just above scans shared buffers so I don't want to just call
> > FlushRelationBuffers() separately.  Instead, I added buffer-flush to
> > SetRelFileNodeBuffersPersistence().
> 
> Maybe I'm missing something, but it sure looks like in the patch that
> SetRelFileNodeBuffersPersistence() is being called after the
> smgrimmedsync() call, and I don't think you get to just switch the order
> of those- the sync is telling the kernel to make sure it's written to
> disk, while the FlushBuffer() is just writing it into the kernel but
> doesn't provide any guarantee that the data has actually made it to
> disk.  We have to FlushBuffer() first, and then call smgrimmedsync().
> Perhaps there's a way to avoid having to go through shared buffers
> twice, and I generally agreed it'd be good if we could avoid doing so,
> but this approach doesn't look like it actually works.

Yeah, sorry for the rare-baked version.. I was confused about the
order at the time.  The next version works like this:

LOGGED->UNLOGGED
<collect reloids to process>

for each relations:
  <set buffer persistence to !BM_PERMANENT (wal-logged if walleve > minimal>
  <create init fork>
  if it is index call ambuildempty() (which syncs the init fork)
  else WAL-log smgr_create then sync the init file.
  <update catalog>
...
commit time:
  <do nogthing>
abort time:
  <unlink init fork>
  <revert buffer persistence>

UNLOGGED->LOGGED
<collect reloids to process>

for each relations:
  <set buffer persistence to !BM_PERMANENT (wal-logged if walleve > minimal>
  <record drop-init-fork to pending-deletes>
  <sync storage files>
  <update catalog>
...
commit time:
  <log smgrunlink>
  <smgrunlink init fork>
abort time:
  <revert buffer persistence>


> > FWIW this is a revised version of the PoC, which has some known
> > problems.
> > 
> > - Flipping of Buffer persistence is not WAL-logged nor even be able to
> >   be safely roll-backed. (It might be better to drop buffers).
> 
> Not sure if it'd be better to drop buffers or not, but figuring out how
> to deal with rollback seems pretty important.  How is the persistence
> change in the catalog not WAL-logged though..?

Rollback works as the above. Buffer persistence change is registered
in pending-deletes.  Persistence change in catalog is rolled back in
the ordinary way (or automatically).

If wal_level > minimal, persistence change of buffers is propagated to
standbys by WAL.  However I'm not sure we need wal-logging otherwise,
the next version emits WAL since SMGR_CREATE is always logged by
existing code.

> > - This version handles indexes but not yet handle toast relatins.
> 
> Would need to be fixed, of course.

Fixed.

> > - tableAMs are supposed to support this feature. (but I'm not sure
> >   it's worth allowing them not to do so).
> 
> Seems like they should.

Init fork of index relations needs a call to ambuildempty() instead of
"log_smgrcreate-smgrimmedsync" after smgrcreate. Instead of adding
similar interface in indexAm, I reverted changes of tableam and make
RelationCreate/DropInitFork() directly do that.  That introduces new
include of amapi.h to storage.c, which is a bit uneasy.

The previous version give up the in-place persistence change in the
case where wal_level > minimal and SET LOGGED since that needs WAL to
be emitted.  However, in-place change still has the advantage of not
running a table copy. So the next verson always runs persistence
change in-place.

As suggested by Andres, I'll send a summary of this patch. The patch
will be attached to the coming mail.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  6. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-13T04:22:01Z

    Hello.  Before posting the next version, I'd like to explain what this
patch is.

1. The Issue

Bulk data loading is a long-time taking, I/O consuming task.  Many
DBAs want that task is faster, even at the cost of increasing risk of
data-loss.  wal_level=minimal is an answer to such a
request. Data-loading onto a table that is created in the current
transaction omits WAL-logging and synced at commit.

However, the optimization doesn't benefit the case where the
data-loading is performed onto existing tables. There are quite a few
cases where data is loaded into tables that already contains a lot of
data. Those cases don't take benefit of the optimization.

Another possible solution for bulk data-loading is UNLOGGED
tables. But when we switch LOGGED/UNLOGGED of a table, all the table
content is copied to a newly created heap, which is costly.


2. Proposed Solutions.

There are two proposed solutions are discussed on this mailing
list. One is wal_level = none (*1), which omits WAL-logging almost at
all. Another is extending the existing optimization to the ALTER TABLE
SET LOGGED/UNLOGGED cases, which is to be discussed in this new
thread.


3. In-place Persistence Change

So the attached is a PoC patch of the "another" solution.  When we
want to change table persistence in-place, basically we need to do the
following steps.

(the talbe is exclusively locked)

(1) Flip BM_PERMANENT flag of all shared buffer blocks for the heap.

(2) Create or delete the init fork for existing heap.

(3) Flush all buffers of the relation to file system.

(4) Sync heap files.

(5) Make catalog changes.


4. Transactionality

The 1, 2 and 5 above need to be abort-able. 5 is rolled back by
existing infrastructure, and rolling-back of 1 and 2 are achieved by
piggybacking on the pendingDeletes mechanism.


5. Replication

Furthermore, that changes ought to be replicable to standbys.  Catalog
changes are replicated as usual. 

On-the-fly creation of the init fork leads to recovery mess. Even
though it is removed at abort, if the server crashed before
transaction end, the file is left alone and corrupts database in the
next recovery. I sought a way to create the init fork in
smgrPendingDelete but that needs relcache and relcache is not
available at that late of commit. Finally, I introduced the fifth fork
kind "INITTMP"(_itmp) only to signal that the init file is not
committed. I don't like that way but it seems working fine...


6. SQL Command

The second file in the patchset adds a syntax that changes persistence
of all tables in a tablespace.

ALTER TABLE ALL IN TABLESPACE <tsp> SET LOGGED/UNLOGGED [ NOWAIT ];


7. Testing

I tried to write TAP test for this, but IPC::Run::harness (or
interactive_psql) doesn't seem to work for me.  I'm not sure what
exactly is happening but pty redirection doesn't work.

  $in = "ls\n"; $out = ""; run ["/usr/bin/bash"], \$in, \$out; print $out;

works but

  $in = "ls\n"; $out = ""; run ["/usr/bin/bash"], '<pty<', \$in, '>pty>', \$out; print $out;

doesn't respond.


The patch is attached.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. RE: In-placre persistance change of a relation

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-11-13T06:43:13Z

    Hi Horiguchi-san,


Thank you for making a patch so quickly.  I've started looking at it.

What makes you think this is a PoC?  Documentation and test cases?  If there's something you think that doesn't work or are concerned about, can you share it?

Do you know the reason why data copy was done before?  And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit.  I can't find it.



(1)
@@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
  */
 #define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
 
+struct SmgrRelationData;

This declaration is already in the file:

/* forward declared, to avoid having to expose buf_internals.h here */
struct WritebackContext;

/* forward declared, to avoid including smgr.h here */
struct SMgrRelationData;


Regards
Takayuki Tsunakawa

  8. RE: In-placre persistance change of a relation

    Takamichi Osumi (Fujitsu) <osumi.takamichi@fujitsu.com> — 2020-11-13T07:15:41Z

    Hello, Tsunakawa-San


> Do you know the reason why data copy was done before?  And, it may be
> odd for me to ask this, but I think I saw someone referred to the past
> discussion that eliminating data copy is difficult due to some processing at
> commit.  I can't find it.
I can share 2 sources why to eliminate the data copy is difficult in hackers thread.

Tom's remark and the context to copy relation's data.
https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#31724.1394163360@sss.pgh.pa.us

Amit-San quoted this thread and mentioned that point in another thread.
https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%40mail.gmail.com

Best,
	Takamichi Osumi

  9. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-13T07:47:48Z

    At Fri, 13 Nov 2020 06:43:13 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> Hi Horiguchi-san,
> 
> 
> Thank you for making a patch so quickly.  I've started looking at it.
> 
> What makes you think this is a PoC?  Documentation and test cases?  If there's something you think that doesn't work or are concerned about, can you share it?

The latest version is heavily revised and is given much comment so it
might have exited from PoC state.  The necessity of documentation is
doubtful since this patch doesn't user-facing behavior other than
speed. Some tests are required especialy about recovery and
replication perspective but I haven't been able to make it. (One of
the tests needs to cause crash while a transaction is running.)

> Do you know the reason why data copy was done before?  And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit.  I can't find it.

To imagine that, just because it is simpler considering rollback and
code sharing, and maybe no one have been complained that SET
LOGGED/UNLOGGED looks taking a long time than required/expected.

The current implement is simple.  It's enough to just discard old or
new relfilenode according to the current transaction ends with commit
or abort. Tweaking of relfilenode under use leads-in some skews in
some places.  I used pendingDelete mechanism a bit complexified way
and a violated an abstraction (I think, calling AM-routines from
storage.c is not good.) and even introduce a new fork kind only to
mark a init fork as "not committed yet".  There might be better way,
but I haven't find it.

(The patch scans all shared buffer blocks for each relation).

> (1)
> @@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
>   */
>  #define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
>  
> +struct SmgrRelationData;
> 
> This declaration is already in the file:
> 
> /* forward declared, to avoid having to expose buf_internals.h here */
> struct WritebackContext;
> 
> /* forward declared, to avoid including smgr.h here */
> struct SMgrRelationData;

Hmmm. Nice chatch. And will fix in the next version.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-11-13T08:23:12Z

    At Fri, 13 Nov 2020 07:15:41 +0000, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> wrote in 
> Hello, Tsunakawa-San
> 

Thanks for sharing it!

> > Do you know the reason why data copy was done before?  And, it may be
> > odd for me to ask this, but I think I saw someone referred to the past
> > discussion that eliminating data copy is difficult due to some processing at
> > commit.  I can't find it.
> I can share 2 sources why to eliminate the data copy is difficult in hackers thread.
> 
> Tom's remark and the context to copy relation's data.
> https://www.postgresql.org/message-id/flat/31724.1394163360%40sss.pgh.pa.us#31724.1394163360@sss.pgh.pa.us

https://www.postgresql.org/message-id/CA+Tgmob44LNwwU73N1aJsGQyzQ61SdhKJRC_89wCm0+aLg=x2Q@mail.gmail.com

> No, not really.  The issue is more around what happens if we crash
> part way through.  At crash recovery time, the system catalogs are not
> available, because the database isn't consistent yet and, anyway, the
> startup process can't be bound to a database, let alone every database
> that might contain unlogged tables.  So the sentinel that's used to
> decide whether to flush the contents of a table or index is the
> presence or absence of an _init fork, which the startup process
> obviously can see just fine.  The _init fork also tells us what to
> stick in the relation when we reset it; for a table, we can just reset
> to an empty file, but that's not legal for indexes, so the _init fork
> contains a pre-initialized empty index that we can just copy over.
> 
> Now, to make an unlogged table logged, you've got to at some stage
> remove those _init forks.  But this is not a transactional operation.
> If you remove the _init forks and then the transaction rolls back,
> you've left the system an inconsistent state.  If you postpone the
> removal until commit time, then you have a problem if it fails,

It's true. That are the cause of headache.

> particularly if it works for the first file but fails for the second.
> And if you crash at any point before you've fsync'd the containing
> directory, you have no idea which files will still be on disk after a
> hard reboot.

This is not an issue in this patch *except* the case where init fork
is failed to removed but the following removal of inittmp fork
succeeds.  Another idea is adding a "not-yet-committed" property to a
fork.  I added a new fork type for easiness of the patch but I could
go that way if that is an issue.

> Amit-San quoted this thread and mentioned that point in another thread.
> https://www.postgresql.org/message-id/CAA4eK1%2BHDqS%2B1fhs5Jf9o4ZujQT%3DXBZ6sU0kOuEh2hqQAC%2Bt%3Dw%40mail.gmail.com

This sounds like a bit differrent discussion. Making part-of-a-table
UNLOGGED looks far difficult to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. RE: In-placre persistance change of a relation

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-12-04T07:49:22Z

    From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> > No, not really.  The issue is more around what happens if we crash
> > part way through.  At crash recovery time, the system catalogs are not
> > available, because the database isn't consistent yet and, anyway, the
> > startup process can't be bound to a database, let alone every database
> > that might contain unlogged tables.  So the sentinel that's used to
> > decide whether to flush the contents of a table or index is the
> > presence or absence of an _init fork, which the startup process
> > obviously can see just fine.  The _init fork also tells us what to
> > stick in the relation when we reset it; for a table, we can just reset
> > to an empty file, but that's not legal for indexes, so the _init fork
> > contains a pre-initialized empty index that we can just copy over.
> >
> > Now, to make an unlogged table logged, you've got to at some stage
> > remove those _init forks.  But this is not a transactional operation.
> > If you remove the _init forks and then the transaction rolls back,
> > you've left the system an inconsistent state.  If you postpone the
> > removal until commit time, then you have a problem if it fails,
> 
> It's true. That are the cause of headache.
...
> The current implement is simple.  It's enough to just discard old or
> new relfilenode according to the current transaction ends with commit
> or abort. Tweaking of relfilenode under use leads-in some skews in
> some places.  I used pendingDelete mechanism a bit complexified way
> and a violated an abstraction (I think, calling AM-routines from
> storage.c is not good.) and even introduce a new fork kind only to
> mark a init fork as "not committed yet".  There might be better way,
> but I haven't find it.

I have no alternative idea yet, too.  I agree that we want to avoid them, especially introducing inittmp fork...  Anyway, below are the rest of my review comments for 0001.  I want to review 0002 when we have decided to go with 0001.


(2)
XLOG_SMGR_UNLINK seems to necessitate modification of the following comments:

[src/include/catalog/storage_xlog.h]
/*
 * Declarations for smgr-related XLOG records
 *
 * Note: we log file creation and truncation here, but logging of deletion
 * actions is handled by xact.c, because it is part of transaction commit.
 */

[src/backend/access/transam/README]
3. Deleting a table, which requires an unlink() that could fail.

Our approach here is to WAL-log the operation first, but to treat failure
of the actual unlink() call as a warning rather than error condition.
Again, this can leave an orphan file behind, but that's cheap compared to
the alternatives.  Since we can't actually do the unlink() until after
we've committed the DROP TABLE transaction, throwing an error would be out
of the question anyway.  (It may be worth noting that the WAL entry about
the file deletion is actually part of the commit record for the dropping
transaction.)


(3)
+/* This is bit-map, not ordianal numbers  */

There seems to be no comments using "bit-map".  "Flags for ..." can be seen here and there.


(4)
Some wrong spellings:

+			/* we flush this buffer when swithing to PERMANENT */

swithing -> switching

+		 * alredy flushed out by RelationCreate(Drop)InitFork called just

alredy -> already

+		 * relation content to be WAL-logged to recovery the table.

recovery -> recover

+	 * The inittmp fork works as the sentinel to identify that situaton.

situaton -> situation


(5)
+	table_close(classRel, NoLock);
+
+
+
+
 }

These empty lines can be deleted.


(6)
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
...
+	 * Make an XLOG entry reporting the file unlink.

Not unlink but buffer persistence?


(7)
+	/*
+	 * index-init fork needs further initialization. ambuildempty shoud do
+	 * WAL-log and file sync by itself but otherwise we do that by myself.
+	 */
+	if (rel->rd_rel->relkind == RELKIND_INDEX)
+		rel->rd_indam->ambuildempty(rel);
+	else
+	{
+		log_smgrcreate(&rnode, INIT_FORKNUM);
+		smgrimmedsync(srel, INIT_FORKNUM);
+	}
+
+	/*
+	 * We have created the init fork. If server crashes before the current
+	 * transaction ends the init fork left alone corrupts data while recovery.
+	 * The inittmp fork works as the sentinel to identify that situaton.
+	 */
+	smgrcreate(srel, INITTMP_FORKNUM, false);
+	log_smgrcreate(&rnode, INITTMP_FORKNUM);
+	smgrimmedsync(srel, INITTMP_FORKNUM);

If the server crashes between these two processings, only the init fork exists.  Is it correct to create the inittmp fork first?


(8)
+	if (inxact_created)
+	{
+		SMgrRelation srel = smgropen(rnode, InvalidBackendId);
+		smgrclose(srel);
+		log_smgrunlink(&rnode, INIT_FORKNUM);
+		smgrunlink(srel, INIT_FORKNUM, false);
	+		log_smgrunlink(&rnode, INITTMP_FORKNUM);
+		smgrunlink(srel, INITTMP_FORKNUM, false);
+		return;
+	}

smgrclose() should be called just before return.
Isn't it  necessary here to revert buffer persistence state change?


(9)
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+	smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}

Maybe it's better to restore smgrdounlinkfork() that was removed in the older release.  That function includes dropping shared buffers, which can clean up the shared buffers that may be cached by this transaction.


(10)
[RelationDropInitFork]
+	/* revert buffer-persistence changes at abort */
+	pending = (PendingRelDelete *)
+		MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+	pending->relnode = rnode;
+	pending->op = PDOP_SET_PERSISTENCE;
+	pending->bufpersistence = false;
+	pending->backend = InvalidBackendId;
+	pending->atCommit = true;
+	pending->nestLevel = GetCurrentTransactionNestLevel();
+	pending->next = pendingDeletes;
+	pendingDeletes = pending;
+}

bufpersistence should be true.


(11)
+				BlockNumber block = 0;
...
+				DropRelFileNodeBuffers(rbnode, &pending->unlink_forknum, 1,
+									   &block);

"block" is unnecessary and 0 can be passed directly.


(12)
-			&& pending->backend == InvalidBackendId)
+			&& pending->backend == InvalidBackendId &&
+			pending->op == PDOP_DELETE)
 			nrels++;

It's better to put && at the beginning of the line to follow the existing code here.


(13)
+	table_close(rel, lockmode);

lockmode should be NoLock to retain the lock until transaction completion.



(14)
+	ctl.keysize = sizeof(unlogged_relation_entry);
+	ctl.entrysize = sizeof(unlogged_relation_entry);
+	hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM);
...
+			memset(key.oid, 0, sizeof(key.oid));
+			memcpy(key.oid, de->d_name, oidchars);
+			ent = hash_search(hash, &key, HASH_FIND, NULL);

keysize should be the oid member of the struct.


Regards
Takayuki Tsunakawa

  12. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-12-24T08:02:20Z

    Thanks for the comment! Sorry for the late reply.

At Fri, 4 Dec 2020 07:49:22 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> > > No, not really.  The issue is more around what happens if we crash
> > > part way through.  At crash recovery time, the system catalogs are not
> > > available, because the database isn't consistent yet and, anyway, the
> > > startup process can't be bound to a database, let alone every database
> > > that might contain unlogged tables.  So the sentinel that's used to
> > > decide whether to flush the contents of a table or index is the
> > > presence or absence of an _init fork, which the startup process
> > > obviously can see just fine.  The _init fork also tells us what to
> > > stick in the relation when we reset it; for a table, we can just reset
> > > to an empty file, but that's not legal for indexes, so the _init fork
> > > contains a pre-initialized empty index that we can just copy over.
> > >
> > > Now, to make an unlogged table logged, you've got to at some stage
> > > remove those _init forks.  But this is not a transactional operation.
> > > If you remove the _init forks and then the transaction rolls back,
> > > you've left the system an inconsistent state.  If you postpone the
> > > removal until commit time, then you have a problem if it fails,
> > 
> > It's true. That are the cause of headache.
> ...
> > The current implement is simple.  It's enough to just discard old or
> > new relfilenode according to the current transaction ends with commit
> > or abort. Tweaking of relfilenode under use leads-in some skews in
> > some places.  I used pendingDelete mechanism a bit complexified way
> > and a violated an abstraction (I think, calling AM-routines from
> > storage.c is not good.) and even introduce a new fork kind only to
> > mark a init fork as "not committed yet".  There might be better way,
> > but I haven't find it.
> 
> I have no alternative idea yet, too.  I agree that we want to avoid them, especially introducing inittmp fork...  Anyway, below are the rest of my review comments for 0001.  I want to review 0002 when we have decided to go with 0001.
> 
> 
> (2)
> XLOG_SMGR_UNLINK seems to necessitate modification of the following comments:
> 
> [src/include/catalog/storage_xlog.h]
> /*
>  * Declarations for smgr-related XLOG records
>  *
>  * Note: we log file creation and truncation here, but logging of deletion
>  * actions is handled by xact.c, because it is part of transaction commit.
>  */

Sure. Rewrote it.

> [src/backend/access/transam/README]
> 3. Deleting a table, which requires an unlink() that could fail.
> 
> Our approach here is to WAL-log the operation first, but to treat failure
> of the actual unlink() call as a warning rather than error condition.
> Again, this can leave an orphan file behind, but that's cheap compared to
> the alternatives.  Since we can't actually do the unlink() until after
> we've committed the DROP TABLE transaction, throwing an error would be out
> of the question anyway.  (It may be worth noting that the WAL entry about
> the file deletion is actually part of the commit record for the dropping
> transaction.)

Mmm. I didn't touched theDROP TABLE (RelationDropStorage) path, but I
added a brief description about INITTMP fork to the file.

====
The INITTMP fork file
--------------------------------

An INITTMP fork is created when new relation file is created to mark
the relfilenode needs to be cleaned up at recovery time. The file is
removed at transaction end but is left when the process crashes before
the transaction ends. In contrast to 4 above, failure to remove an
INITTMP file will lead to data loss, in which case the server will
shut down.
====

> (3)
> +/* This is bit-map, not ordianal numbers  */
> 
> There seems to be no comments using "bit-map".  "Flags for ..." can be seen here and there.

I revmoed the comment and use (1 << n) notation to show the fact
instead.


> (4)
> Some wrong spellings:
> 
> swithing -> switching
> alredy -> already
> recovery -> recover
> situaton -> situation

Oops! Fixed them.

> (5)
> +	table_close(classRel, NoLock);
> +
> +
> +
> +
>  }
> 
> These empty lines can be deleted.

s/can/should/ :p. Fixed.

> 
> (6)
> +/*
> + * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
> + */
> +void
> +log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
> ...
> +	 * Make an XLOG entry reporting the file unlink.
> 
> Not unlink but buffer persistence?

Silly copy-pasto. Fixed.

> (7)
> +	/*
> +	 * index-init fork needs further initialization. ambuildempty shoud do
> +	 * WAL-log and file sync by itself but otherwise we do that by myself.
> +	 */
> +	if (rel->rd_rel->relkind == RELKIND_INDEX)
> +		rel->rd_indam->ambuildempty(rel);
> +	else
> +	{
> +		log_smgrcreate(&rnode, INIT_FORKNUM);
> +		smgrimmedsync(srel, INIT_FORKNUM);
> +	}
> +
> +	/*
> +	 * We have created the init fork. If server crashes before the current
> +	 * transaction ends the init fork left alone corrupts data while recovery.
> +	 * The inittmp fork works as the sentinel to identify that situaton.
> +	 */
> +	smgrcreate(srel, INITTMP_FORKNUM, false);
> +	log_smgrcreate(&rnode, INITTMP_FORKNUM);
> +	smgrimmedsync(srel, INITTMP_FORKNUM);
> 
> If the server crashes between these two processings, only the init fork exists.  Is it correct to create the inittmp fork first?

Right. I change it that way, and did the same with the new code added
to RelationCreateStorage.

> (8)
> +	if (inxact_created)
> +	{
> +		SMgrRelation srel = smgropen(rnode, InvalidBackendId);
> +		smgrclose(srel);
> +		log_smgrunlink(&rnode, INIT_FORKNUM);
> +		smgrunlink(srel, INIT_FORKNUM, false);
> 	+		log_smgrunlink(&rnode, INITTMP_FORKNUM);
> +		smgrunlink(srel, INITTMP_FORKNUM, false);
> +		return;
> +	}
> 
> smgrclose() should be called just before return.
> Isn't it  necessary here to revert buffer persistence state change?

Mmm. it's a thinko. I was confused with the case of
close/unlink. Fixed all instacnes of the same.

> (9)
> +void
> +smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
> +{
> +	smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
> +}
> 
> Maybe it's better to restore smgrdounlinkfork() that was removed in the older release.  That function includes dropping shared buffers, which can clean up the shared buffers that may be cached by this transaction.

INITFORK/INITTMP forks cannot be loaded to shared buffer so it's no
use to drop buffers.  I added a comment like that.

|	/*
|	 * INIT/INITTMP forks never be loaded to shared buffer so no point in
|	 * dropping buffers for these files.
|	 */
|	log_smgrunlink(&rnode, INIT_FORKNUM);

I removed DropRelFileNodeBuffers from PDOP_UNLINK_FORK branch in
smgrDoPendingDeletes and added an assertion and a comment instead.

|		/* other forks needs to drop buffers */
|		Assert(pending->unlink_forknum == INIT_FORKNUM ||
|			   pending->unlink_forknum == INITTMP_FORKNUM);
|
|		log_smgrunlink(&pending->relnode, pending->unlink_forknum);
|		smgrunlink(srel, pending->unlink_forknum, false);


> (10)
> [RelationDropInitFork]
> +	/* revert buffer-persistence changes at abort */
> +	pending = (PendingRelDelete *)
> +		MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
> +	pending->relnode = rnode;
> +	pending->op = PDOP_SET_PERSISTENCE;
> +	pending->bufpersistence = false;
> +	pending->backend = InvalidBackendId;
> +	pending->atCommit = true;
> +	pending->nestLevel = GetCurrentTransactionNestLevel();
> +	pending->next = pendingDeletes;
> +	pendingDeletes = pending;
> +}
> 
> bufpersistence should be true.

RelationDropInitFork() chnages the relation persisitence to
"persistent" so it shoud be reverted to "non-persistent (= false)" at
abort. (I agree that the function name is somewhat confusing...)


> (11)
> +				BlockNumber block = 0;
> ...
> +				DropRelFileNodeBuffers(rbnode, &pending->unlink_forknum, 1,
> +									   &block);
> 
> "block" is unnecessary and 0 can be passed directly.

I removed the entire function call.

But, I don't think you're right here.

| DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber *forkNum,
| 					   int nforks, BlockNumber *firstDelBlock)

Doesn't just passing 0 lead to SEGV?

> (12)
> -			&& pending->backend == InvalidBackendId)
> +			&& pending->backend == InvalidBackendId &&
> +			pending->op == PDOP_DELETE)
>  			nrels++;
> 
> It's better to put && at the beginning of the line to follow the existing code here.

It's terrible.. Fixed.


> (13)
> +	table_close(rel, lockmode);
> 
> lockmode should be NoLock to retain the lock until transaction completion.

I tried to recall the reason for that, but didn't come up with
anything. Fixed.

> (14)
> +	ctl.keysize = sizeof(unlogged_relation_entry);
> +	ctl.entrysize = sizeof(unlogged_relation_entry);
> +	hash = hash_create("unlogged hash", 32, &ctl, HASH_ELEM);
> ...
> +			memset(key.oid, 0, sizeof(key.oid));
> +			memcpy(key.oid, de->d_name, oidchars);
> +			ent = hash_search(hash, &key, HASH_FIND, NULL);
> 
> keysize should be the oid member of the struct.

It's not a problem since the first member is the oid and perhaps it
seems that I thougth to do someting more on that.  Now that I don't
recall what is it and in the first place the key should be just Oid in
the context above. Fixed.

The patch is attached to the next message.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-12-25T00:12:52Z

    Hello.

At Thu, 24 Dec 2020 17:02:20 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> The patch is attached to the next message.

The reason for separating this message is that I modified this so that
it could solve another issue.

There's a complain about orphan files after crash. [1]

1: https://www.postgresql.org/message-id/16771-cbef7d97ba93f4b9@postgresql.org

That is, the case where a relation file is left alone after a server
crash that happened before the end of the transaction that has created
a relation.  As I read this, I noticed this feature can solve the
issue with a small change.

This version gets changes in RelationCreateStorage and
smgrDoPendingDeletes.

Previously inittmp fork is created only along with an init fork. This
version creates one always when a relation storage file is created. As
the result ResetUnloggedRelationsInDbspaceDir removes all forks if the
inttmp fork of a logged relations is found.  Now that pendingDeletes
can contain multiple entries for the same relation, it has been
modified not to close the same smgr multiple times.

- It might be better to split 0001 into two peaces.

- The function name ResetUnloggedRelationsInDbspaceDir is no longer
  represents the function correctly.
	  
regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  14. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-08T05:47:05Z

    At Fri, 25 Dec 2020 09:12:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Hello.
> 
> At Thu, 24 Dec 2020 17:02:20 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > The patch is attached to the next message.
> 
> The reason for separating this message is that I modified this so that
> it could solve another issue.
> 
> There's a complain about orphan files after crash. [1]
> 
> 1: https://www.postgresql.org/message-id/16771-cbef7d97ba93f4b9@postgresql.org
> 
> That is, the case where a relation file is left alone after a server
> crash that happened before the end of the transaction that has created
> a relation.  As I read this, I noticed this feature can solve the
> issue with a small change.
> 
> This version gets changes in RelationCreateStorage and
> smgrDoPendingDeletes.
> 
> Previously inittmp fork is created only along with an init fork. This
> version creates one always when a relation storage file is created. As
> the result ResetUnloggedRelationsInDbspaceDir removes all forks if the
> inttmp fork of a logged relations is found.  Now that pendingDeletes
> can contain multiple entries for the same relation, it has been
> modified not to close the same smgr multiple times.
> 
> - It might be better to split 0001 into two peaces.
> 
> - The function name ResetUnloggedRelationsInDbspaceDir is no longer
>   represents the function correctly.

As pointed by Robert in another thread [1], persisntence of (at least)
GiST index cannot be flipped in-place due to incompatibility of fake
LSNs with real ones.

This version RelationChangePersistence() is changed not to choose
in-place method for indexes other than btree. It seems to be usable
with all kind of indexes other than Gist, but at the mement it applies
only to btrees.

1: https://www.postgresql.org/message-id/CA+TgmoZEZ5RONS49C7mEpjhjndqMQtVrz_LCQUkpRWdmRevDnQ@mail.gmail.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-08T08:52:21Z

    At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> This version RelationChangePersistence() is changed not to choose
> in-place method for indexes other than btree. It seems to be usable
> with all kind of indexes other than Gist, but at the mement it applies
> only to btrees.
> 
> 1: https://www.postgresql.org/message-id/CA+TgmoZEZ5RONS49C7mEpjhjndqMQtVrz_LCQUkpRWdmRevDnQ@mail.gmail.com

Hmm. This is not wroking correctly. I'll repost after fixint that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-12T09:58:08Z

    At Fri, 08 Jan 2021 17:52:21 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > This version RelationChangePersistence() is changed not to choose
> > in-place method for indexes other than btree. It seems to be usable
> > with all kind of indexes other than Gist, but at the mement it applies
> > only to btrees.
> > 
> > 1: https://www.postgresql.org/message-id/CA+TgmoZEZ5RONS49C7mEpjhjndqMQtVrz_LCQUkpRWdmRevDnQ@mail.gmail.com
> 
> Hmm. This is not wroking correctly. I'll repost after fixint that.

I think I fixed the misbehavior. ResetUnloggedRelationsInDbspaceDir()
handles file operations in the wrong order and with the wrong logic.
It also needed to drop buffers and forget fsync requests.

I thought that the two cases that this patch is expected to fix
(orphan relation files and uncommited init files) can share the same
"cleanup" fork but that is wrong. I had to add one more additional
fork to differentiate the cases of SET UNLOGGED and of creation of
UNLOGGED tables...

The attached is a new version, that seems working correctly but looks
somewhat messy. I'll continue working.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  17. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-14T08:32:17Z

    At Tue, 12 Jan 2021 18:58:08 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 08 Jan 2021 17:52:21 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > At Fri, 08 Jan 2021 14:47:05 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > > This version RelationChangePersistence() is changed not to choose
> > > in-place method for indexes other than btree. It seems to be usable
> > > with all kind of indexes other than Gist, but at the mement it applies
> > > only to btrees.
> > > 
> > > 1: https://www.postgresql.org/message-id/CA+TgmoZEZ5RONS49C7mEpjhjndqMQtVrz_LCQUkpRWdmRevDnQ@mail.gmail.com
> > 
> > Hmm. This is not wroking correctly. I'll repost after fixint that.
> 
> I think I fixed the misbehavior. ResetUnloggedRelationsInDbspaceDir()
> handles file operations in the wrong order and with the wrong logic.
> It also needed to drop buffers and forget fsync requests.
> 
> I thought that the two cases that this patch is expected to fix
> (orphan relation files and uncommited init files) can share the same
> "cleanup" fork but that is wrong. I had to add one more additional
> fork to differentiate the cases of SET UNLOGGED and of creation of
> UNLOGGED tables...
> 
> The attached is a new version, that seems working correctly but looks
> somewhat messy. I'll continue working.

Commit bea449c635 conflicts with this on the change of the definition
of DropRelFileNodeBuffers. The change simplified this patch by a bit:p

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  18. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-03-25T05:08:05Z

    (I'm not sure when the subject was broken..)

At Thu, 14 Jan 2021 17:32:17 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Commit bea449c635 conflicts with this on the change of the definition
> of DropRelFileNodeBuffers. The change simplified this patch by a bit:p

In this version, I got rid of the "CLEANUP FORK"s, and added a new
system "Smgr marks".  The mark files have the name of the
corresponding fork file followed by ".u" (which means Uncommitted.).
"Uncommited"-marked main fork means the same as the CLEANUP2_FORKNUM
and uncommitted-marked init fork means the same as the CLEANUP_FORKNUM
in the previous version.x

I noticed that the previous version of the patch still leaves an
orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
revmoed at rollback.  In this version the responsibility to remove the
mark files is moved to SyncPostCheckpoint, where main fork files are
actually removed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  19. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-03-25T05:15:03Z

    At Thu, 25 Mar 2021 14:08:05 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> (I'm not sure when the subject was broken..)
> 
> At Thu, 14 Jan 2021 17:32:17 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > Commit bea449c635 conflicts with this on the change of the definition
> > of DropRelFileNodeBuffers. The change simplified this patch by a bit:p
> 
> In this version, I got rid of the "CLEANUP FORK"s, and added a new
> system "Smgr marks".  The mark files have the name of the
> corresponding fork file followed by ".u" (which means Uncommitted.).
> "Uncommited"-marked main fork means the same as the CLEANUP2_FORKNUM
> and uncommitted-marked init fork means the same as the CLEANUP_FORKNUM
> in the previous version.x
> 
> I noticed that the previous version of the patch still leaves an
> orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
> before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
> revmoed at rollback.  In this version the responsibility to remove the
> mark files is moved to SyncPostCheckpoint, where main fork files are
> actually removed.

For the record, I noticed that basebackup could be confused by the
mark files but I haven't looked that yet.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-17T09:10:30Z

    > Kyotaro wrote:
> > In this version, I got rid of the "CLEANUP FORK"s, and added a new
> > system "Smgr marks".  The mark files have the name of the
> > corresponding fork file followed by ".u" (which means Uncommitted.).
> > "Uncommited"-marked main fork means the same as the
> CLEANUP2_FORKNUM
> > and uncommitted-marked init fork means the same as the
> CLEANUP_FORKNUM
> > in the previous version.x
> >
> > I noticed that the previous version of the patch still leaves an
> > orphan main fork file after "BEGIN; CREATE TABLE x; ROLLBACK; (crash
> > before checkpoint)" since the "mark" file (or CLEANUP2_FORKNUM) is
> > revmoed at rollback.  In this version the responsibility to remove the
> > mark files is moved to SyncPostCheckpoint, where main fork files are
> > actually removed.
> 
> For the record, I noticed that basebackup could be confused by the mark files
> but I haven't looked that yet.
> 

Good morning Kyotaro,

the patch didn't apply clean (it's from March; some hunks were failing), so I've fixed it and the combined git format-patch is attached. It did conflict with the following:
	b0483263dda - Add support for SET ACCESS METHOD in ALTER TABLE
	7b565843a94 - Add call to object access hook at the end of table rewrite in ALTER TABLE
	9ce346eabf3 - Report progress of startup operations that take a long time.
	f10f0ae420 - Replace RelationOpenSmgr() with RelationGetSmgr().

I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".

Basic demonstration of this patch (with wal_level=minimal):
	create unlogged table t6 (id bigint, t text);
	-- produces 110GB table, takes ~5mins
	insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100000000);
	alter table t6 set logged;
		on baseline SET LOGGED takes: ~7min10s
		on patched SET LOGGED takes: 25s

So basically one can - thanks to this patch - use his application (performing classic INSERTs/UPDATEs/DELETEs, so without the need to rewrite to use COPY) and perform literally batch upload and then just switch the tables to LOGGED. 

Some more intensive testing also looks good, assuming table prepared to put pressure on WAL:
	create unlogged table t_unlogged (id bigint, t text) partition by hash (id);
	create unlogged table t_unlogged_h0 partition of t_unlogged  FOR VALUES WITH (modulus 4, remainder 0);
	[..]
	create unlogged table t_unlogged_h3 partition of t_unlogged  FOR VALUES WITH (modulus 4, remainder 3);

Workload would still be pretty heavy on LWLock/BufferContent,WALInsert and Lock/extend .
	t_logged.sql = insert into t_logged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~1MB of WAL
	t_unlogged.sql = insert into t_unlogged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~3kB of WAL

so using: pgbench -f <tabletypetest>.sql -T 30 -P 1 -c 32 -j 3 t 
with synchronous_commit =ON(default):
	with t_logged.sql:   tps = 229 (lat avg = 138ms)
	with t_unlogged.sql  tps = 283 (lat avg = 112ms) # almost all on LWLock/WALWrite
with synchronous_commit =OFF:
	with t_logged.sql:   tps = 413 (lat avg = 77ms)
	with t_unloged.sql:  tps = 782 (lat avg = 40ms)
Afterwards switching the unlogged ~16GB partitions takes 5s per partition. 

As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state. 
I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.

-Jakub Wartak.

  21. Re: In-placre persistance change of a relation

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-17T12:46:38Z

    On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".

> As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state. 
> I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.

The patch is failing:
http://cfbot.cputube.org/kyotaro-horiguchi.html
https://api.cirrus-ci.com/v1/artifact/task/5564333871595520/regress_diffs/src/bin/pg_upgrade/tmp_check/regress/regression.diffs

I think you ran "make check", but should run something like this:
make check-world -j8 >check-world.log 2>&1 && echo Success

-- 
Justin

  22. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-17T14:33:25Z

    >  Justin wrote:
> On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> > As the thread didn't get a lot of traction, I've registered it into current
> commitfest
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommitf
> est.postgresql.org%2F36%2F3461%2F&amp;data=04%7C01%7CJakub.Wartak%
> 40tomtom.com%7Cb815e75090d44e20fd0a08d9c15b45cc%7C374f80267b544a
> 3ab87d328fa26ec10d%7C0%7C0%7C637753420044612362%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C3000&amp;sdata=0BTQSVDnVPu4YpECKXXlBJT5q3Gfgv099SaC
> NuBwiW4%3D&amp;reserved=0 with You as the author and in 'Ready for
> review' state.
> 
> The patch is failing:
[..] 
> I think you ran "make check", but should run something like this:
> make check-world -j8 >check-world.log 2>&1 && echo Success

Hi Justin,

I've repeated the check-world and it says to me all is ok locally (also with --enable-cassert --enable-debug , at least on Amazon Linux 2) and also installcheck on default params seems to be ok  
I don't seem to understand why testfarm reports errors for tests like "path, polygon, rowsecurity" e.g. on Linux/graviton2 and FreeBSD while not on MacOS(?) . 
Could someone point to me where to start looking/fixing?

-J.

  23. Re: In-placre persistance change of a relation

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-17T19:10:28Z

    On Fri, Dec 17, 2021 at 02:33:25PM +0000, Jakub Wartak wrote:
> >  Justin wrote:
> > On Fri, Dec 17, 2021 at 09:10:30AM +0000, Jakub Wartak wrote:
> > > As the thread didn't get a lot of traction, I've registered it into current
> > commitfest
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcommitf
> > est.postgresql.org%2F36%2F3461%2F&amp;data=04%7C01%7CJakub.Wartak%
> > 40tomtom.com%7Cb815e75090d44e20fd0a08d9c15b45cc%7C374f80267b544a
> > 3ab87d328fa26ec10d%7C0%7C0%7C637753420044612362%7CUnknown%7CT
> > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> > CI6Mn0%3D%7C3000&amp;sdata=0BTQSVDnVPu4YpECKXXlBJT5q3Gfgv099SaC
> > NuBwiW4%3D&amp;reserved=0 with You as the author and in 'Ready for
> > review' state.
> > 
> > The patch is failing:
> [..] 
> > I think you ran "make check", but should run something like this:
> > make check-world -j8 >check-world.log 2>&1 && echo Success
> 
> Hi Justin,
> 
> I've repeated the check-world and it says to me all is ok locally (also with --enable-cassert --enable-debug , at least on Amazon Linux 2) and also installcheck on default params seems to be ok  
> I don't seem to understand why testfarm reports errors for tests like "path, polygon, rowsecurity" e.g. on Linux/graviton2 and FreeBSD while not on MacOS(?) . 
> Could someone point to me where to start looking/fixing?

Since it says this, it looks a lot like a memory error like a use-after-free
 - like in fsync_parent_path():

 CREATE TABLE PATH_TBL (f1 path);
+ERROR:  could not open file <....>  Pacific": No such file or directory

I see at least this one is still failing, though:
time make -C src/test/recovery check

  24. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-20T06:28:23Z

    Hello, Jakub.

At Fri, 17 Dec 2021 09:10:30 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> the patch didn't apply clean (it's from March; some hunks were failing), so I've fixed it and the combined git format-patch is attached. It did conflict with the following:

Thanks for looking this. Also thanks for Justin for finding the silly
use-after-free bug. (Now I see the regression test fails and I'm not
sure how come I didn't find this before.)

> 	b0483263dda - Add support for SET ACCESS METHOD in ALTER TABLE
> 	7b565843a94 - Add call to object access hook at the end of table rewrite in ALTER TABLE
> 	9ce346eabf3 - Report progress of startup operations that take a long time.
> 	f10f0ae420 - Replace RelationOpenSmgr() with RelationGetSmgr().
> 
> I'm especially worried if I didn't screw up something/forgot something related to the last one (rd->rd_smgr changes), but I'm getting "All 210 tests passed".

About the last one, all rel->rd_smgr acesses need to be repalced with
RelationGetSmgr(). On the other hand we can simply remove
RelationOpenSmgr() calls since the target smgrrelation is guaranteed
to be loaded by RelationGetSmgr().

The fix you made for RelationCreate/DropInitFork is correct and
changes you made would work, but I prefer that the code not being too
permissive for unknown (or unexpected) states.

> Basic demonstration of this patch (with wal_level=minimal):
> 	create unlogged table t6 (id bigint, t text);
> 	-- produces 110GB table, takes ~5mins
> 	insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100000000);
> 	alter table t6 set logged;
> 		on baseline SET LOGGED takes: ~7min10s
> 		on patched SET LOGGED takes: 25s
> 
> So basically one can - thanks to this patch - use his application (performing classic INSERTs/UPDATEs/DELETEs, so without the need to rewrite to use COPY) and perform literally batch upload and then just switch the tables to LOGGED. 

This result is significant. That operation finally requires WAL writes
but I was not sure how much gain FPIs (or bulk WAL logging) gives in
comparison to operational WALs.

> Some more intensive testing also looks good, assuming table prepared to put pressure on WAL:
> 	create unlogged table t_unlogged (id bigint, t text) partition by hash (id);
> 	create unlogged table t_unlogged_h0 partition of t_unlogged  FOR VALUES WITH (modulus 4, remainder 0);
> 	[..]
> 	create unlogged table t_unlogged_h3 partition of t_unlogged  FOR VALUES WITH (modulus 4, remainder 3);
> 
> Workload would still be pretty heavy on LWLock/BufferContent,WALInsert and Lock/extend .
> 	t_logged.sql = insert into t_logged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~1MB of WAL
> 	t_unlogged.sql = insert into t_unlogged select nextval('s1'), repeat('A', 1000) from generate_series(1, 1000); # according to pg_wal_stats.wal_bytes generates ~3kB of WAL
> 
> so using: pgbench -f <tabletypetest>.sql -T 30 -P 1 -c 32 -j 3 t 
> with synchronous_commit =ON(default):
> 	with t_logged.sql:   tps = 229 (lat avg = 138ms)
> 	with t_unlogged.sql  tps = 283 (lat avg = 112ms) # almost all on LWLock/WALWrite
> with synchronous_commit =OFF:
> 	with t_logged.sql:   tps = 413 (lat avg = 77ms)
> 	with t_unloged.sql:  tps = 782 (lat avg = 40ms)
> Afterwards switching the unlogged ~16GB partitions takes 5s per partition. 
> 
> As the thread didn't get a lot of traction, I've registered it into current commitfest https://commitfest.postgresql.org/36/3461/ with You as the author and in 'Ready for review' state.
> 
> I think it behaves as almost finished one and apparently after reading all those discussions that go back over 10years+ time span about this feature, and lot of failed effort towards wal_level=noWAL I think it would be nice to finally start getting some of that of it into the core.

Thanks for taking the performance benchmark.

I didn't register for some reasons.

1. I'm not sure that we want to have the new mark files.

2. Aside of possible bugs, I'm not confident that the crash-safety of
  this patch is actually water-tight. At least we need tests for some
  failure cases.

3. As mentioned in transam/README, failure in removing smgr mark files
   leads to immediate shut down.  I'm not sure this behavior is acceptable.

4. Including the reasons above, this is not fully functionally.
   For example, if we execute the following commands on primary,
   replica dones't work correctly. (boom!)

   =# CREATE UNLOGGED TABLE t (a int);
   =# ALTER TABLE t SET LOGGED;


The following fixes are done in the attched v8.

- Rebased. Referring to Jakub and Justin's work, I replaced direct
  access to ->rd_smgr with RelationGetSmgr() and removed calls to
  RelationOpenSmgr(). I still separate the "ALTER TABLE ALL IN
  TABLESPACE SET LOGGED/UNLOGGED" statement part.

- Fixed RelationCreate/DropInitFork's behavior for non-target
  relations. (From Jakub's work).

- Fixed wording of some comments.

- As revisited, I found a bug around recovery. If the logged-ness of a
  relation gets flipped repeatedly in a transaction, duplicate
  pending-delete entries are accumulated during recovery and work in a
  wrong way. sgmr_redo now adds up to one entry for a action.

- The issue 4 above is not fixed (yet).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  25. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-20T07:53:20Z

    At Mon, 20 Dec 2021 15:28:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> 
> 4. Including the reasons above, this is not fully functionally.
>    For example, if we execute the following commands on primary,
>    replica dones't work correctly. (boom!)
> 
>    =# CREATE UNLOGGED TABLE t (a int);
>    =# ALTER TABLE t SET LOGGED;

> - The issue 4 above is not fixed (yet).

Not only for the case, RelationChangePersistence needs to send a
truncate record before FPIs.  If primary crashes amid of the
operation, the table content will be vanish with the persistence
change. That is the correct behavior.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  26. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-20T07:59:29Z

    Hi Kyotaro, I'm glad you are still into this

> I didn't register for some reasons.

Right now in v8 there's a typo in ./src/backend/catalog/storage.c :

storage.c: In function 'RelationDropInitFork':
storage.c:385:44: error: expected statement before ')' token
    pending->unlink_forknum != INIT_FORKNUM)) <-- here, one ) too much

> 1. I'm not sure that we want to have the new mark files.

I can't help with such design decision, but if there are doubts maybe then add checking return codes around:
a) pg_fsync() and fsync_parent_path() (??) inside mdcreatemark()
b) mdunlinkmark() inside mdunlinkmark()
and PANIC if something goes wrong?

> 2. Aside of possible bugs, I'm not confident that the crash-safety of
>  this patch is actually water-tight. At least we need tests for some
>  failure cases.
>
> 3. As mentioned in transam/README, failure in removing smgr mark files
>    leads to immediate shut down.  I'm not sure this behavior is acceptable.

Doesn't it happen for most of the stuff already? There's even data_sync_retry GUC.

> 4. Including the reasons above, this is not fully functionally.
>    For example, if we execute the following commands on primary,
>    replica dones't work correctly. (boom!)
> 
>    =# CREATE UNLOGGED TABLE t (a int);
>    =# ALTER TABLE t SET LOGGED;
> 
> 
> The following fixes are done in the attched v8.
> 
> - Rebased. Referring to Jakub and Justin's work, I replaced direct
>   access to ->rd_smgr with RelationGetSmgr() and removed calls to
>   RelationOpenSmgr(). I still separate the "ALTER TABLE ALL IN
>   TABLESPACE SET LOGGED/UNLOGGED" statement part.
> 
> - Fixed RelationCreate/DropInitFork's behavior for non-target
>   relations. (From Jakub's work).
> 
> - Fixed wording of some comments.
> 
> - As revisited, I found a bug around recovery. If the logged-ness of a
>   relation gets flipped repeatedly in a transaction, duplicate
>   pending-delete entries are accumulated during recovery and work in a
>   wrong way. sgmr_redo now adds up to one entry for a action.
> 
> - The issue 4 above is not fixed (yet).

Thanks again, If you have any list of crush tests ideas maybe I'll have some minutes 
to try to figure them out. Is there is any goto list of stuff to be checked to add confidence
to this patch (as per point #2) ?

BTW fast feedback regarding that ALTER patch  (there were 4 unlogged tables):
#  ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
WARNING:  unrecognized node type: 349

-J.

  27. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-20T08:39:27Z

    At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> Hi Kyotaro, I'm glad you are still into this
> 
> > I didn't register for some reasons.
> 
> Right now in v8 there's a typo in ./src/backend/catalog/storage.c :
> 
> storage.c: In function 'RelationDropInitFork':
> storage.c:385:44: error: expected statement before ')' token
>     pending->unlink_forknum != INIT_FORKNUM)) <-- here, one ) too much

Yeah, I thought that I had removed it. v9 patch I believe is correct.

> > 1. I'm not sure that we want to have the new mark files.
> 
> I can't help with such design decision, but if there are doubts maybe then add checking return codes around:
> a) pg_fsync() and fsync_parent_path() (??) inside mdcreatemark()
> b) mdunlinkmark() inside mdunlinkmark()
> and PANIC if something goes wrong?

The point is it is worth the complexity it adds. Since the mark file
can resolve another existing (but I don't recall in detail) issue and
this patchset actually fixes it, it can be said to have a certain
extent of persuasiveness. But that doesn't change the fact that it's
additional complexity.

> > 2. Aside of possible bugs, I'm not confident that the crash-safety of
> >  this patch is actually water-tight. At least we need tests for some
> >  failure cases.
> >
> > 3. As mentioned in transam/README, failure in removing smgr mark files
> >    leads to immediate shut down.  I'm not sure this behavior is acceptable.
> 
> Doesn't it happen for most of the stuff already? There's even data_sync_retry GUC.

Hmm. Yes, actually it is "as water-tight as possible".  I just want
others' eyes on that perspective.  CF could be the entry point of
others but I'm a bit hesitent to add a new entry..

> > 4. Including the reasons above, this is not fully functionally.
> >    For example, if we execute the following commands on primary,
> >    replica dones't work correctly. (boom!)
> > 
> >    =# CREATE UNLOGGED TABLE t (a int);
> >    =# ALTER TABLE t SET LOGGED;
> > 
> > 
> > The following fixes are done in the attched v8.
> > 
> > - Rebased. Referring to Jakub and Justin's work, I replaced direct
> >   access to ->rd_smgr with RelationGetSmgr() and removed calls to
> >   RelationOpenSmgr(). I still separate the "ALTER TABLE ALL IN
> >   TABLESPACE SET LOGGED/UNLOGGED" statement part.
> > 
> > - Fixed RelationCreate/DropInitFork's behavior for non-target
> >   relations. (From Jakub's work).
> > 
> > - Fixed wording of some comments.
> > 
> > - As revisited, I found a bug around recovery. If the logged-ness of a
> >   relation gets flipped repeatedly in a transaction, duplicate
> >   pending-delete entries are accumulated during recovery and work in a
> >   wrong way. sgmr_redo now adds up to one entry for a action.
> > 
> > - The issue 4 above is not fixed (yet).
> 
> Thanks again, If you have any list of crush tests ideas maybe I'll have some minutes 
> to try to figure them out. Is there is any goto list of stuff to be checked to add confidence
> to this patch (as per point #2) ?

Just causing a crash (kill -9) after executing problem-prone command
sequence, then seeing recovery works well would sufficient.

For example:

create unlogged table; begin; insert ..; alter table set logged;
<crash>.  Recovery works.

"create logged; begin; {alter unlogged; alter logged;} * 1000; alter
logged; commit/abort" doesn't pollute pgdata.


> BTW fast feedback regarding that ALTER patch  (there were 4 unlogged tables):
> #  ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
> WARNING:  unrecognized node type: 349

lol  I met a server crash. Will fix. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  28. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-20T08:52:21Z

    At Mon, 20 Dec 2021 17:39:27 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> > BTW fast feedback regarding that ALTER patch  (there were 4 unlogged tables):
> > #  ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
> > WARNING:  unrecognized node type: 349
> 
> lol  I met a server crash. Will fix. Thanks!

That crash vanished after a recompilation for me and I don't see that
error.  On my dev env node# 349 is T_ALterTableSetLoggedAllStmt, which
0002 adds.  So perhaps make clean/make all would fix that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  29. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-20T13:38:35Z

    Hi Kyotaro,

> At Mon, 20 Dec 2021 17:39:27 +0900 (JST), Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote in
> > At Mon, 20 Dec 2021 07:59:29 +0000, Jakub Wartak
> > <Jakub.Wartak@tomtom.com> wrote in
> > > BTW fast feedback regarding that ALTER patch  (there were 4 unlogged
> tables):
> > > #  ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
> > > WARNING:  unrecognized node type: 349
> >
> > lol  I met a server crash. Will fix. Thanks!
> 
> That crash vanished after a recompilation for me and I don't see that error.  On
> my dev env node# 349 is T_ALterTableSetLoggedAllStmt, which
> 0002 adds.  So perhaps make clean/make all would fix that.

The fastest I could - I've repeated the whole cycle about that one with fresh v9 (make clean, configure, make install, fresh initdb) and I've found two problems:

1) check-worlds seems OK but make -C src/test/recovery check shows a couple of failing tests here locally and in https://cirrus-ci.com/task/4699985735319552?logs=test#L807 :
t/009_twophase.pl                  (Wstat: 256 Tests: 24 Failed: 1)
  Failed test:  21
  Non-zero exit status: 1
t/014_unlogged_reinit.pl           (Wstat: 512 Tests: 12 Failed: 2)
  Failed tests:  9-10
  Non-zero exit status: 2
t/018_wal_optimize.pl              (Wstat: 7424 Tests: 0 Failed: 0)
  Non-zero exit status: 29
  Parse errors: Bad plan.  You planned 38 tests but ran 0.
t/022_crash_temp_files.pl          (Wstat: 7424 Tests: 6 Failed: 0)
  Non-zero exit status: 29
  Parse errors: Bad plan.  You planned 9 tests but ran 6.

018 made no sense, I've tried to take a quick look with wal_level=minimal why it is failing , it is mystery to me as the sequence seems to be pretty basic but the outcome is not:
~> cat repro.sql
create tablespace tbs1 location '/tbs1';
CREATE TABLE moved (id int);
INSERT INTO moved VALUES (1);
BEGIN;
ALTER TABLE moved SET TABLESPACE tbs1;
CREATE TABLE originated (id int);
INSERT INTO originated VALUES (1);
CREATE UNIQUE INDEX ON originated(id) TABLESPACE tbs1;
COMMIT;

~> psql -f repro.sql z3; sleep 1; /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
CREATE TABLESPACE
CREATE TABLE
INSERT 0 1
BEGIN
ALTER TABLE
CREATE TABLE
INSERT 0 1
CREATE INDEX
COMMIT
waiting for server to shut down.... done
server stopped
~> /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
waiting for server to start.... done
server started
z3# select * from moved;
ERROR:  could not open file "pg_tblspc/32834/PG_15_202112131/32833/32838": No such file or directory
z3=# select * from originated;
ERROR:  could not open file "base/32833/32839": No such file or directory
z3=# \dt+
                              List of relations
 Schema |    Name    | Type  |  Owner   | Persistence |  Size   | Description
--------+------------+-------+----------+-------------+---------+-------------
 public | moved      | table | postgres | permanent   | 0 bytes |
 public | originated | table | postgres | permanent   | 0 bytes |

This happens even without placing on tablespace at all {for originated table , but no for moved on}, some major mishap is there (commit should guarantee correctness) or I'm tired and having sloppy fingers.

2) minor one testcase, still something is odd.

drop tablespace tbs1;
create tablespace tbs1 location '/tbs1';
CREATE UNLOGGED TABLE t4 (a int) tablespace tbs1;
CREATE UNLOGGED TABLE t5 (a int) tablespace tbs1;
CREATE UNLOGGED TABLE t6 (a int) tablespace tbs1;
CREATE TABLE t7 (a int) tablespace tbs1;
insert into t7 values (1);
insert into t5 values (1);
insert into t6 values (1);
\dt+
                             List of relations
 Schema | Name | Type  |  Owner   | Persistence |    Size    | Description
--------+------+-------+----------+-------------+------------+-------------
 public | t4   | table | postgres | unlogged    | 0 bytes    |
 public | t5   | table | postgres | unlogged    | 8192 bytes |
 public | t6   | table | postgres | unlogged    | 8192 bytes |
 public | t7   | table | postgres | permanent   | 8192 bytes |
(4 rows)

ALTER TABLE ALL IN TABLESPACE tbs1 set logged; 
==> STILL WARNING:  unrecognized node type: 349
\dt+
                             List of relations
 Schema | Name | Type  |  Owner   | Persistence |    Size    | Description
--------+------+-------+----------+-------------+------------+-------------
 public | t4   | table | postgres | permanent   | 0 bytes    |
 public | t5   | table | postgres | permanent   | 8192 bytes |
 public | t6   | table | postgres | permanent   | 8192 bytes |
 public | t7   | table | postgres | permanent   | 8192 bytes |

So it did rewrite however this warning seems to be unfixed. I've tested on e2c52beecdea152ca680a22ef35c6a7da55aa30f.

-J.

  30. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-21T08:13:21Z

    Ugh! I completely forgot about TAP tests.. Thanks for the testing and
sorry for the bugs.

This is a bit big change so I need a bit of time before posting the
next version.


At Mon, 20 Dec 2021 13:38:35 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> 1) check-worlds seems OK but make -C src/test/recovery check shows a couple of failing tests here locally and in https://cirrus-ci.com/task/4699985735319552?logs=test#L807 :
> t/009_twophase.pl                  (Wstat: 256 Tests: 24 Failed: 1)
>   Failed test:  21
>   Non-zero exit status: 1

PREPARE TRANSACTION requires uncommited file creation to be
committed. Concretely we need to remove the "mark" files for the
in-transaction created relation file during PREPARE TRANSACTION.

pendingSync is not a parallel mechanism with pendingDeletes so we
cannot move mark deletion to pendingSync.

After all I decided to add a separate list pendingCleanups for pending
non-deletion tasks separately from pendingDeletes and execute it
before insering the commit record.  Not only the above but also all of
the following failures vanished by the change.

> t/014_unlogged_reinit.pl           (Wstat: 512 Tests: 12 Failed: 2)
>   Failed tests:  9-10
>   Non-zero exit status: 2
> t/018_wal_optimize.pl              (Wstat: 7424 Tests: 0 Failed: 0)
>   Non-zero exit status: 29
>   Parse errors: Bad plan.  You planned 38 tests but ran 0.
> t/022_crash_temp_files.pl          (Wstat: 7424 Tests: 6 Failed: 0)
>   Non-zero exit status: 29
>   Parse errors: Bad plan.  You planned 9 tests but ran 6.


> 018 made no sense, I've tried to take a quick look with wal_level=minimal why it is failing , it is mystery to me as the sequence seems to be pretty basic but the outcome is not:

I think this shares the same cause.

> ~> cat repro.sql
> create tablespace tbs1 location '/tbs1';
> CREATE TABLE moved (id int);
> INSERT INTO moved VALUES (1);
> BEGIN;
> ALTER TABLE moved SET TABLESPACE tbs1;
> CREATE TABLE originated (id int);
> INSERT INTO originated VALUES (1);
> CREATE UNIQUE INDEX ON originated(id) TABLESPACE tbs1;
> COMMIT;
..
> ERROR:  could not open file "base/32833/32839": No such file or directory


> z3=# \dt+
>                               List of relations
>  Schema |    Name    | Type  |  Owner   | Persistence |  Size   | Description
> --------+------------+-------+----------+-------------+---------+-------------
>  public | moved      | table | postgres | permanent   | 0 bytes |
>  public | originated | table | postgres | permanent   | 0 bytes |
> 
> This happens even without placing on tablespace at all {for originated table , but no for moved on}, some major mishap is there (commit should guarantee correctness) or I'm tired and having sloppy fingers.
> 
> 2) minor one testcase, still something is odd.
> 
> drop tablespace tbs1;
> create tablespace tbs1 location '/tbs1';
> CREATE UNLOGGED TABLE t4 (a int) tablespace tbs1;
> CREATE UNLOGGED TABLE t5 (a int) tablespace tbs1;
> CREATE UNLOGGED TABLE t6 (a int) tablespace tbs1;
> CREATE TABLE t7 (a int) tablespace tbs1;
> insert into t7 values (1);
> insert into t5 values (1);
> insert into t6 values (1);
> \dt+
>                              List of relations
>  Schema | Name | Type  |  Owner   | Persistence |    Size    | Description
> --------+------+-------+----------+-------------+------------+-------------
>  public | t4   | table | postgres | unlogged    | 0 bytes    |
>  public | t5   | table | postgres | unlogged    | 8192 bytes |
>  public | t6   | table | postgres | unlogged    | 8192 bytes |
>  public | t7   | table | postgres | permanent   | 8192 bytes |
> (4 rows)
> 
> ALTER TABLE ALL IN TABLESPACE tbs1 set logged; 
> ==> STILL WARNING:  unrecognized node type: 349
> \dt+
>                              List of relations
>  Schema | Name | Type  |  Owner   | Persistence |    Size    | Description
> --------+------+-------+----------+-------------+------------+-------------
>  public | t4   | table | postgres | permanent   | 0 bytes    |
>  public | t5   | table | postgres | permanent   | 8192 bytes |
>  public | t6   | table | postgres | permanent   | 8192 bytes |
>  public | t7   | table | postgres | permanent   | 8192 bytes |
> 
> So it did rewrite however this warning seems to be unfixed. I've tested on e2c52beecdea152ca680a22ef35c6a7da55aa30f.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  31. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-21T11:04:55Z

    At Tue, 21 Dec 2021 17:13:21 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Ugh! I completely forgot about TAP tests.. Thanks for the testing and
> sorry for the bugs.
> 
> This is a bit big change so I need a bit of time before posting the
> next version.

I took a bit too long detour but the patch gets to pass make-world for
me.

In this version:

- When relation persistence is changed from logged to unlogged, buffer
 persistence is flipped then an init-fork is created along with a mark
 file for the fork (RelationCreateInitFork). The mark file is removed
 at commit but left alone after a crash before commit. At the next
 startup, ResetUnloggedRelationsInDbspaceDir() removes the init fork
 file if it finds the mark file corresponding to the file.

- When relation persistence is changed from unlogged to logged, buffer
  persistence is flipped then the exisging init-fork is marked to be
  dropped at commit (RelationDropInitFork). Finally the whole content
  is WAL-logged in the page-wise manner (RelationChangePersistence),

- The two operations above are repeatable within a transaction and
 commit makes the last operation persist and rollback make the all
 operations abandoned.

- Storage files are created along with a "mark" file for the
 relfilenode. It behaves the same way to the above except the mark
 files corresponds to the whole relfilenode.

- The at-commit operations this patch adds require to be WAL-logged so
 they don't fit pendingDeletes list, which is executed after commit. I
 added a new pending-work list pendingCleanups that is executed just
 after pendingSyncs.  (new in this version)

 
regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  32. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-21T13:07:28Z

    Hi Kyotaro,

> I took a bit too long detour but the patch gets to pass make-world for me.

Good news, v10 passes all the tests for me (including TAP recover ones). There's major problem I think:

drop table t6;
create unlogged table t6 (id bigint, t text);
create sequence s1;
insert into t6 select nextval('s1'), repeat('A', 1000) from generate_series(1, 100);
alter table t6 set logged;
select pg_sleep(1);
<--optional checkpoint, more on this later.
/usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
/usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
select count(*) from t6; -- shows 0 rows

But If I perform checkpoint before crash, data is there..  apparently the missing steps done by checkpointer 
seem to help. If checkpoint is not done, then some peeking reveals that upon startup there is truncation (?!):

$ /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile -m immediate stop
$ find /var/lib/pgsql/15/data/ -name '73741*' -ls
112723206  120 -rw-------   1 postgres postgres   122880 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741
112723202   24 -rw-------   1 postgres postgres    24576 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741_fsm
$ /usr/pgsql-15/bin/pg_ctl -D /var/lib/pgsql/15/data -l logfile start
waiting for server to start.... done
server started
$ find /var/lib/pgsql/15/data/ -name '73741*' -ls
112723206    0 -rw-------   1 postgres postgres        0 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741
112723202   16 -rw-------   1 postgres postgres    16384 Dec 21 12:42 /var/lib/pgsql/15/data/base/73740/73741_fsm

So what's suspicious is that 122880 -> 0 file size truncation. I've investigated WAL and it seems to contain TRUNCATE records
after logged FPI images, so when the crash recovery would kick in it probably clears this table (while it shouldn't).

However if I perform CHECKPOINT just before crash the WAL stream contains just RUNNING_XACTS and CHECKPOINT_ONLINE 
redo records, this probably prevents truncating. I'm newbie here so please take this theory with grain of salt, it can be 
something completely different.

-J.

  33. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-22T06:13:27Z

    Hello, Jakub.

At Tue, 21 Dec 2021 13:07:28 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> So what's suspicious is that 122880 -> 0 file size truncation. I've investigated WAL and it seems to contain TRUNCATE records
> after logged FPI images, so when the crash recovery would kick in it probably clears this table (while it shouldn't).

Darn..  It is too silly that I wrongly issued truncate records for the
target relation of the function (rel) instaed of the relation on which
we're currently operating at that time (r).

> However if I perform CHECKPOINT just before crash the WAL stream contains just RUNNING_XACTS and CHECKPOINT_ONLINE 
> redo records, this probably prevents truncating. I'm newbie here so please take this theory with grain of salt, it can be 
> something completely different.

It is because the WAL records are inconsistent with the on-disk state.
After a crash before a checkpoint after the SET LOGGED, recovery ends with
recoverying the broken WAL records, but after that the on-disk state
is persisted and the broken WAL records are not replayed.

The following fix works.

--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5478,7 +5478,7 @@ RelationChangePersistence(AlteredTableInfo *tab, char persistence,
                        xl_smgr_truncate xlrec;
 
                        xlrec.blkno = 0;
-                       xlrec.rnode = rel->rd_node;
+                       xlrec.rnode = r->rd_node;
                        xlrec.flags = SMGR_TRUNCATE_ALL;
 

I made another change in this version. Previously only btree among all
index AMs was processed in the in-place manner.  In this version we do
that all AMs except GiST.  Maybe if gistGetFakeLSN behaved the same
way for permanent and unlogged indexes, we could skip index rebuild in
exchange of some extra WAL records emitted while it is unlogged.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  34. RE: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2021-12-22T08:42:14Z

    Hi Kyotaro,

> At Tue, 21 Dec 2021 13:07:28 +0000, Jakub Wartak
> <Jakub.Wartak@tomtom.com> wrote in
> > So what's suspicious is that 122880 -> 0 file size truncation. I've
> > investigated WAL and it seems to contain TRUNCATE records after logged
> FPI images, so when the crash recovery would kick in it probably clears this
> table (while it shouldn't).
> 
> Darn..  It is too silly that I wrongly issued truncate records for the target
> relation of the function (rel) instaed of the relation on which we're currently
> operating at that time (r).
> 
> [..]
> The following fix works.

Cool, I have verified basic stuff that was coming to my mind, now even cfbot is happy with v11, You should happy too I hope :)

> I made another change in this version. Previously only btree among all index
> AMs was processed in the in-place manner.  In this version we do that all
> AMs except GiST.  Maybe if gistGetFakeLSN behaved the same way for
> permanent and unlogged indexes, we could skip index rebuild in exchange of
> some extra WAL records emitted while it is unlogged.

I think there's slight omission:

-- unlogged table -> logged with GiST:
DROP TABLE IF EXISTS testcase;
CREATE UNLOGGED TABLE testcase(geom geometry not null);
CREATE INDEX idx_testcase_gist ON testcase USING gist(geom);
INSERT INTO testcase(geom) SELECT ST_Buffer(ST_SetSRID(ST_MakePoint(-1.0, 2.0),4326), 0.0001);
ALTER TABLE testcase SET LOGGED;

-- crashes with:
(gdb) where
#0  reindex_index (indexId=indexId@entry=65541, skip_constraint_checks=skip_constraint_checks@entry=true, persistence=persistence@entry=112 'p', params=params@entry=0x0) at index.c:3521
#1  0x000000000062f494 in RelationChangePersistence (tab=tab@entry=0x1947258, persistence=112 'p', lockmode=lockmode@entry=8) at tablecmds.c:5434
#2  0x0000000000642819 in ATRewriteTables (context=0x7ffc19c04520, lockmode=<optimized out>, wqueue=0x7ffc19c04388, parsetree=0x1925ec8) at tablecmds.c:5644
[..]
#10 0x00000000007f078f in exec_simple_query (query_string=0x1925340 "ALTER TABLE testcase SET LOGGED;") at postgres.c:1215

apparently reindex_index() params cannot be NULL - the same happens with switching persistent 
table to unlogged one too (with GiST). 

I'll also try to give another shot to the patch early next year - as we are starting long Christmas/holiday break here 
- with verifying WAL for GiST and more advanced setup (more crashes, and standby/archiving/barman to see 
how it's possible to use wal_level=minimal <-> replica transitions). 

-J.

  35. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-23T06:01:41Z

    At Wed, 22 Dec 2021 08:42:14 +0000, Jakub Wartak <Jakub.Wartak@tomtom.com> wrote in 
> I think there's slight omission:
...
> apparently reindex_index() params cannot be NULL - the same happens with switching persistent

Hmm. a3dc926009 has changed the interface. (But the name is also
changed after that.)

-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexParams *params)

> I'll also try to give another shot to the patch early next year - as we are starting long Christmas/holiday break here 
> - with verifying WAL for GiST and more advanced setup (more crashes, and standby/archiving/barman to see 
> how it's possible to use wal_level=minimal <-> replica transitions). 

Thanks. I added TAP test to excecise the in-place persistence change.

have a nice holiday, Jakub!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  36. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-23T06:33:35Z

    At Thu, 23 Dec 2021 15:01:41 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I added TAP test to excecise the in-place persistence change.

We don't need a base table for every index. TAP test revised.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  37. Re: In-placre persistance change of a relation

    Andres Freund <andres@anarazel.de> — 2022-01-05T00:05:08Z

    On 2021-12-23 15:33:35 +0900, Kyotaro Horiguchi wrote:
> At Thu, 23 Dec 2021 15:01:41 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > I added TAP test to excecise the in-place persistence change.
>
> We don't need a base table for every index. TAP test revised.

The tap tests seems to fail on all platforms. See
https://cirrus-ci.com/build/4911549314760704

E.g. the linux failure is

[16:45:15.569]
[16:45:15.569] #   Failed test 'inserted'
[16:45:15.569] #   at t/027_persistence_change.pl line 121.
[16:45:15.569] # Looks like you failed 1 test of 25.
[16:45:15.569] [16:45:15] t/027_persistence_change.pl ..........
[16:45:15.569] Dubious, test returned 1 (wstat 256, 0x100)
[16:45:15.569] Failed 1/25 subtests
[16:45:15.569] [16:45:15]
[16:45:15.569]
[16:45:15.569] Test Summary Report
[16:45:15.569] -------------------
[16:45:15.569] t/027_persistence_change.pl        (Wstat: 256 Tests: 25 Failed: 1)
[16:45:15.569]   Failed test:  18
[16:45:15.569]   Non-zero exit status: 1
[16:45:15.569] Files=27, Tests=315, 220 wallclock secs ( 0.14 usr  0.03 sys + 48.94 cusr 17.13 csys = 66.24 CPU)

https://api.cirrus-ci.com/v1/artifact/task/4785083130314752/tap/src/test/recovery/tmp_check/log/regress_log_027_persistence_change

Greetings,

Andres Freund

  38. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-06T04:30:17Z

    At Tue, 4 Jan 2022 16:05:08 -0800, Andres Freund <andres@anarazel.de> wrote in 
> The tap tests seems to fail on all platforms. See
> https://cirrus-ci.com/build/4911549314760704
> 
> E.g. the linux failure is
> 
> [16:45:15.569]
> [16:45:15.569] #   Failed test 'inserted'
> [16:45:15.569] #   at t/027_persistence_change.pl line 121.
> [16:45:15.569] # Looks like you failed 1 test of 25.
> [16:45:15.569] [16:45:15] t/027_persistence_change.pl ..........
> [16:45:15.569] Dubious, test returned 1 (wstat 256, 0x100)
> [16:45:15.569] Failed 1/25 subtests
> [16:45:15.569] [16:45:15]
> [16:45:15.569]
> [16:45:15.569] Test Summary Report
> [16:45:15.569] -------------------
> [16:45:15.569] t/027_persistence_change.pl        (Wstat: 256 Tests: 25 Failed: 1)
> [16:45:15.569]   Failed test:  18
> [16:45:15.569]   Non-zero exit status: 1
> [16:45:15.569] Files=27, Tests=315, 220 wallclock secs ( 0.14 usr  0.03 sys + 48.94 cusr 17.13 csys = 66.24 CPU)
> 
> https://api.cirrus-ci.com/v1/artifact/task/4785083130314752/tap/src/test/recovery/tmp_check/log/regress_log_027_persistence_change

Thank you very much. It still doesn't fail on my devlopment
environment (CentOS8), but I found a silly bug of the test script.
I'm still not sure the reason the test item failed but I repost the
updated version then watch what the CI says.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  39. Re: In-placre persistance change of a relation

    Andres Freund <andres@anarazel.de> — 2022-01-06T04:42:32Z

    Hi, 

On January 5, 2022 8:30:17 PM PST, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>At Tue, 4 Jan 2022 16:05:08 -0800, Andres Freund <andres@anarazel.de> wrote in 
>> The tap tests seems to fail on all platforms. See
>> https://cirrus-ci.com/build/4911549314760704
>> 
>> E.g. the linux failure is
>> 
>> [16:45:15.569]
>> [16:45:15.569] #   Failed test 'inserted'
>> [16:45:15.569] #   at t/027_persistence_change.pl line 121.
>> [16:45:15.569] # Looks like you failed 1 test of 25.
>> [16:45:15.569] [16:45:15] t/027_persistence_change.pl ..........
>> [16:45:15.569] Dubious, test returned 1 (wstat 256, 0x100)
>> [16:45:15.569] Failed 1/25 subtests
>> [16:45:15.569] [16:45:15]
>> [16:45:15.569]
>> [16:45:15.569] Test Summary Report
>> [16:45:15.569] -------------------
>> [16:45:15.569] t/027_persistence_change.pl        (Wstat: 256 Tests: 25 Failed: 1)
>> [16:45:15.569]   Failed test:  18
>> [16:45:15.569]   Non-zero exit status: 1
>> [16:45:15.569] Files=27, Tests=315, 220 wallclock secs ( 0.14 usr  0.03 sys + 48.94 cusr 17.13 csys = 66.24 CPU)
>> 
>> https://api.cirrus-ci.com/v1/artifact/task/4785083130314752/tap/src/test/recovery/tmp_check/log/regress_log_027_persistence_change
>
>Thank you very much. It still doesn't fail on my devlopment
>environment (CentOS8), but I found a silly bug of the test script.
>I'm still not sure the reason the test item failed but I repost the
>updated version then watch what the CI says.

Fwiw, you can now test the same way as cfbot does with a lower turnaround time, as explained in src/tools/ci/README
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  40. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-06T07:39:21Z

    At Wed, 05 Jan 2022 20:42:32 -0800, Andres Freund <andres@anarazel.de> wrote in 
> Hi, 
> 
> On January 5, 2022 8:30:17 PM PST, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >I'm still not sure the reason the test item failed but I repost the
> >updated version then watch what the CI says.
> 
> Fwiw, you can now test the same way as cfbot does with a lower turnaround time, as explained in src/tools/ci/README

Fantastic! I'll give it a try. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  41. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-07T08:29:55Z

    At Thu, 06 Jan 2022 16:39:21 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Fantastic! I'll give it a try. Thanks!

I did that and found that the test stumbled on newlines.
Tests succeeded for other than Windows.

Windows version fails for a real known issue.

[7916][postmaster] LOG:  received immediate shutdown request
[7916][postmaster] LOG:  database system is shut down
[6228][postmaster] LOG:  starting PostgreSQL 15devel, compiled by Visual C++ build 1929, 64-bit
[6228][postmaster] LOG:  listening on Unix socket "C:/Users/ContainerAdministrator/AppData/Local/Temp/NcMnt2KTsr/.s.PGSQL.58698"
[2948][startup] LOG:  database system was interrupted; last known up at 2022-01-07 07:12:14 GMT
[2948][startup] LOG:  database system was not properly shut down; automatic recovery in progress
[2948][startup] LOG:  redo starts at 0/1484280
[2948][startup] LOG:  invalid record length at 0/14A47B8: wanted 24, got 0
[2948][startup] FATAL:  could not remove file "base/12759/16384.u": Permission denied
[6228][postmaster] LOG:  startup process (PID 2948) exited with exit code 1


Mmm.. Someone is still grasping the file after restart?

Anyway, I post the fixed version.  This still fails on Windows..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  42. Re: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@tomtom.com> — 2022-01-11T09:33:55Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

I've retested v15 of the patch with everything that came to my mind. The patch passes all my tests (well, there's this just windows / cfbot issue). Patch looks good to me. I haven't looked in-depth at the code, so patch might still need review.

FYI, about potential usage of this patch: the most advanced test that I did was continually bouncing wal_level - it works. So chain of :
1. wal_level=replica->minimal
2. alter table set unlogged and load a lot of data, set logged
3. wal_level=minimal->replica
4. barman incremental backup # rsync(1) just backups changed files in steps 2 and 3 (not whole DB)
5. some other (logged) work
The idea is that when performing mass-alterations to the DB (think nightly ETL/ELT on TB-sized DBs), one could skip backing up most of DB and then just quickly backup only the changed files - during the maintenance window - e.g. thanks to local-rsync barman mode. This is the output of barman show-backups after loading data to unlogged table each such cycle:
mydb 20220110T100236 - Mon Jan 10 10:05:14 2022 - Size: 144.1 GiB - WAL Size: 16.0 KiB
mydb 20220110T094905 - Mon Jan 10 09:50:12 2022 - Size: 128.5 GiB - WAL Size: 80.2 KiB
mydb 20220110T094016 - Mon Jan 10 09:40:17 2022 - Size: 109.1 GiB - WAL Size: 496.3 KiB
And dedupe ratio of the last one: Backup size: 144.1 GiB. Actual size on disk: 36.1 GiB (-74.96% deduplication ratio).  

The only thing I've found out that bouncing wal_level also forces max_wal_senders=X -> 0 -> X which in turn requires dropping replication slot for pg_receievewal (e.g. barman receive-wal --create-slot/--drop-slot/--reset). I have tested the restore using barman recover afterwards to backup 20220110T094905  and indeed it worked OK using this patch too.

The new status of this patch is: Needs review

  43. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-14T02:43:10Z

    I found a bug.

mdmarkexists() didn't close the tentatively opend fd. This is a silent
leak on Linux and similars and it causes delete failure on Windows.
It was the reason of the CI failure.

027_persistence_change.pl uses interactive_psql() that doesn't work on
the Windos VM on the CI.

In this version the following changes have been made in 0001.

- Properly close file descriptor in mdmarkexists.

- Skip some tests when IO::Pty is not available.
  It might be better to separate that part.

Looking again the ALTER TABLE ALL IN TABLESPACE SET LOGGED patch, I
noticed that it doesn't implement OWNED BY part and doesn't have test
and documenttaion (it was PoC). Added all of them to 0002.

At Tue, 11 Jan 2022 09:33:55 +0000, Jakub Wartak <jakub.wartak@tomtom.com> wrote in 
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
> 
> I've retested v15 of the patch with everything that came to my mind. The patch passes all my tests (well, there's this just windows / cfbot issue). Patch looks good to me. I haven't looked in-depth at the code, so patch might still need review.

Thanks for checking.

> FYI, about potential usage of this patch: the most advanced test that I did was continually bouncing wal_level - it works. So chain of :
> 1. wal_level=replica->minimal
> 2. alter table set unlogged and load a lot of data, set logged
> 3. wal_level=minimal->replica
> 4. barman incremental backup # rsync(1) just backups changed files in steps 2 and 3 (not whole DB)
> 5. some other (logged) work
> The idea is that when performing mass-alterations to the DB (think nightly ETL/ELT on TB-sized DBs), one could skip backing up most of DB and then just quickly backup only the changed files - during the maintenance window - e.g. thanks to local-rsync barman mode. This is the output of barman show-backups after loading data to unlogged table each such cycle:
> mydb 20220110T100236 - Mon Jan 10 10:05:14 2022 - Size: 144.1 GiB - WAL Size: 16.0 KiB
> mydb 20220110T094905 - Mon Jan 10 09:50:12 2022 - Size: 128.5 GiB - WAL Size: 80.2 KiB
> mydb 20220110T094016 - Mon Jan 10 09:40:17 2022 - Size: 109.1 GiB - WAL Size: 496.3 KiB
> And dedupe ratio of the last one: Backup size: 144.1 GiB. Actual size on disk: 36.1 GiB (-74.96% deduplication ratio).  

Ah, The patch skips duping relation files. This is advantageous that
that not only eliminates the I/O activities the duping causes but also
reduce the size of incremental backup.  I didn't noticed only the
latter advantage.

> The only thing I've found out that bouncing wal_level also forces max_wal_senders=X -> 0 -> X which in turn requires dropping replication slot for pg_receievewal (e.g. barman receive-wal --create-slot/--drop-slot/--reset). I have tested the restore using barman recover afterwards to backup 20220110T094905  and indeed it worked OK using this patch too.

Year, it is irrelevant to this patch but I'm annoyed by the
restriction.  I think it would be okay that max_wal_senders is
forcibly set to 0 while wal_level=minimal..

> The new status of this patch is: Needs review

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  44. Re: In-placre persistance change of a relation

    Julien Rouhaud <rjuju123@gmail.com> — 2022-01-18T06:26:31Z

    Hi,

On Fri, Jan 14, 2022 at 11:43:10AM +0900, Kyotaro Horiguchi wrote:
> I found a bug.
> 
> mdmarkexists() didn't close the tentatively opend fd. This is a silent
> leak on Linux and similars and it causes delete failure on Windows.
> It was the reason of the CI failure.
> 
> 027_persistence_change.pl uses interactive_psql() that doesn't work on
> the Windos VM on the CI.
> 
> In this version the following changes have been made in 0001.
> 
> - Properly close file descriptor in mdmarkexists.
> 
> - Skip some tests when IO::Pty is not available.
>   It might be better to separate that part.
> 
> Looking again the ALTER TABLE ALL IN TABLESPACE SET LOGGED patch, I
> noticed that it doesn't implement OWNED BY part and doesn't have test
> and documenttaion (it was PoC). Added all of them to 0002.

The cfbot is failing on all OS with this version of the patch.  Apparently
v16-0002 introduces some usage of "testtablespace" client-side variable that's
never defined, e.g.
https://api.cirrus-ci.com/v1/artifact/task/4670105480069120/regress_diffs/src/bin/pg_upgrade/tmp_check/regress/regression.diffs:

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/tablespace.out /tmp/cirrus-ci-build/src/bin/pg_upgrade/tmp_check/regress/results/tablespace.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/tablespace.out	2022-01-18 04:26:38.744707547 +0000
+++ /tmp/cirrus-ci-build/src/bin/pg_upgrade/tmp_check/regress/results/tablespace.out	2022-01-18 04:30:37.557078083 +0000
@@ -948,76 +948,71 @@
 CREATE SCHEMA testschema;
 GRANT CREATE ON SCHEMA testschema TO regress_tablespace_user1;
 CREATE TABLESPACE regress_tablespace LOCATION :'testtablespace';
+ERROR:  syntax error at or near ":"
+LINE 1: CREATE TABLESPACE regress_tablespace LOCATION :'testtablespa...

  45. Re: In-placre persistance change of a relation

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-01-18T15:37:53Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> The cfbot is failing on all OS with this version of the patch.  Apparently
> v16-0002 introduces some usage of "testtablespace" client-side variable that's
> never defined, e.g.

That test infrastructure got rearranged very recently, see d6d317dbf.

			regards, tom lane

  46. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-01-19T00:39:07Z

    At Tue, 18 Jan 2022 10:37:53 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > The cfbot is failing on all OS with this version of the patch.  Apparently
> > v16-0002 introduces some usage of "testtablespace" client-side variable that's
> > never defined, e.g.
> 
> That test infrastructure got rearranged very recently, see d6d317dbf.

Thanks to both. It seems that even though I know about the change, I
forgot to make my repo up to date before checking.

The v17 attached changes only the following point (as well as
corresponding "expected" file).

-+CREATE TABLESPACE regress_tablespace LOCATION :'testtablespace';
++CREATE TABLESPACE regress_tablespace LOCATION '';

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  47. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-01T05:14:13Z

    Rebased on a recent xlog refactoring.

No functional changes have been made.

- Removed the default case in smgr_desc since it seems to me we don't
 assume out-of-definition values in xlog records elsewhere.

- Simplified some added to storage.c.

- Fix copy-pasto'ed comments in extractPageInfo().

- The previous version smgrDoPendingCleanups() assumes that init-fork
  are not loaded onto shared buffer but it is wrong
  (SetRelationBuffersPersistence assumes the opposite.).  Thus we need
  to drop buffers before unlink an init fork. But it is already
  guaranteed by logic so I rewrote the comment for for PCOP_UNLINK_FORK.

  > * Unlink the fork file. Currently we use this only for
  > * init forks and we're sure that the init fork is not
  > * loaded on shared buffers.  For RelationDropInitFork
  > * case, the function dropped that buffers. For
  > * RelationCreateInitFork case, PCOP_SET_PERSISTENCE(true)
  > * is set and the buffers have been dropped just before.
  
  This logic has the same critical window as
  DropRelFilenodeBuffers. That is, if file deletion fails after
  successful buffer dropping, theoretically the file content of the
  init fork may be stale. However, AFAICS init-fork is write-once fork
  so I don't think that actually matters.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  48. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-01T08:50:30Z

    At Tue, 01 Mar 2022 14:14:13 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> - Removed the default case in smgr_desc since it seems to me we don't
>  assume out-of-definition values in xlog records elsewhere.

Stupid.  The complier on the CI environemnt complains for
uninitialized variable even though it (presumably) knows that the all
paths of the switch statement set the variable.  Added default value
to try to silence compiler.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  49. Re: In-placre persistance change of a relation

    Justin Pryzby <pryzby@telsasoft.com> — 2022-03-30T13:44:02Z

    On Tue, Mar 01, 2022 at 02:14:13PM +0900, Kyotaro Horiguchi wrote:
> Rebased on a recent xlog refactoring.

It'll come as no surprise that this neds to be rebased again.
At least a few typos I reported in January aren't fixed.
Set to "waiting".

  50. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-31T04:58:45Z

    Thanks! Version 20 is attached.


At Wed, 30 Mar 2022 08:44:02 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> On Tue, Mar 01, 2022 at 02:14:13PM +0900, Kyotaro Horiguchi wrote:
> > Rebased on a recent xlog refactoring.
> 
> It'll come as no surprise that this neds to be rebased again.
> At least a few typos I reported in January aren't fixed.
> Set to "waiting".

Oh, I'm sorry for overlooking it. It somehow didn't show up on my
mailer.

> I started looking at this and reviewed docs and comments again.
> 
> > +typedef struct PendingCleanup
> > +{
> > +	RelFileNode relnode;		/* relation that may need to be deleted */
> > +	int			op;				/* operation mask */
> > +	bool		bufpersistence;	/* buffer persistence to set */
> > +	int			unlink_forknum;	/* forknum to unlink */
> 
> This can be of data type "ForkNumber"

Right. Fixed.

> > +	 * We are going to create an init fork. If server crashes before the
> > +	 * current transaction ends the init fork left alone corrupts data while
> > +	 * recovery.  The mark file works as the sentinel to identify that
> > +	 * situation.
> 
> s/while/during/

This was in v17, but dissapeared in v18.

> > +	 * index-init fork needs further initialization. ambuildempty shoud do
> 
> should (I reported this before)
> 
> > +	if (inxact_created)
> > +	{
> > +		SMgrRelation srel = smgropen(rnode, InvalidBackendId);
> > +
> > +		/*
> > +		 * INIT forks never be loaded to shared buffer so no point in dropping
> 
> "are never loaded"

If was fixed in v18.

> > +	elog(DEBUG1, "perform in-place persistnce change");
> 
> persistence (I reported this before)

Sorry. Fixed.

> > +		/*
> > +		 * While wal_level >= replica, switching to LOGGED requires the
> > +		 * relation content to be WAL-logged to recover the table.
> > +		 * We don't emit this fhile wal_level = minimal.
> 
> while (or "if")

There are "While" and "fhile". I changed the latter to "if".

> > +			 * The relation is persistent and stays remain persistent. Don't
> > +			 * drop the buffers for this relation.
> 
> "stays remain" is redundant (I reported this before)

Thanks. I changed it to "stays persistent".

> > +			if (unlink(rm_path) < 0)
> > +				ereport(ERROR,
> > +						(errcode_for_file_access(),
> > +						 errmsg("could not remove file \"%s\": %m",
> > +								rm_path)));
> 
> The parens around errcode are unnecessary since last year.
> I suggest to avoid using them here and elsewhere.

It is just moved from elsewhere without editing, but of course I can
do that.  I didn't know about that change of ereport and not found the
corresponding commit, but I found that Tom mentioned that change.

https://www.postgresql.org/message-id/flat/5063.1584641224%40sss.pgh.pa.us#63e611c30800133bbddb48de857668e8
> Now that we can rely on having varargs macros, I think we could
> stop requiring the extra level of parentheses, ie instead of
...
>         ereport(ERROR,
>                 errcode(ERRCODE_DIVISION_BY_ZERO),
>                 errmsg("division by zero"));
> 
> (The old syntax had better still work, of course.  I'm not advocating
> running around and changing existing calls.)

I changed all ereport calls added by this patch to this style.

> > +			 * And we may have SMGR_MARK_UNCOMMITTED file.  Remove it if the
> > +			 * fork files has been successfully removed. It's ok if the file
> 
> file

Fixed.

> > +     <para>
> > +      All tables in the current database in a tablespace can be changed by using
> 
> given tablespace

I did /database in a tablespace/database in the given tablespace/. Is
it right?

> > +      the <literal>ALL IN TABLESPACE</literal> form, which will lock all tables
> 
> which will first lock
>
> > +      to be changed first and then change each one.  This form also supports
> 
> remove "first" here

This is almost a dead copy of the description of SET TABLESPACE. This
change makes the two almost the same description vary slightly in that
wordings.  Anyway I did that as suggested only for the part this patch
adds in this version.

> > +      <literal>OWNED BY</literal>, which will only change tables owned by the
> > +      roles specified.  If the <literal>NOWAIT</literal> option is specified
> 
> specified roles.
> is specified, (comma)

This is the same as above. I did that but it makes the description
differ from another almost-the-same description.

> > +      then the command will fail if it is unable to acquire all of the locks
> 
> if it is unable to immediately acquire
>
> > +      required immediately.  The <literal>information_schema</literal>
> 
> remove immediately

Ditto.

> > +      relations are not considered part of the system catalogs and will be
> 
> I think you need to first say that "relations in the pg_catalog schema cannot
> be changed".

Mmm. I don't agree on this.  Aren't such "exceptions"-ish descriptions
usually placed after the descriptions of how the feature works? This
is also the same structure with SET TABLESPACE.

> in patch 2/2:
> typo: persistene

Hmm. Bad. I checked the spellings of the whole patches and found some
typos.

+					 * The crashed trasaction did SET UNLOGGED. This relation
+					 * is restored to a LOGGED relation.
s/trasaction/transaction/

+				errmsg("could not crete mark file \"%s\": %m", path));
s/crete/create/

Then rebased on 9c08aea6a3 then pgindent'ed.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  51. Re: In-placre persistance change of a relation

    Justin Pryzby <pryzby@telsasoft.com> — 2022-03-31T05:37:07Z

    On Thu, Mar 31, 2022 at 01:58:45PM +0900, Kyotaro Horiguchi wrote:
> Thanks! Version 20 is attached.

The patch failed an all CI tasks, and seems to have caused the macos task to
hang.

http://cfbot.cputube.org/kyotaro-horiguchi.html

Would you send a fixed patch, or remove this thread from the CFBOT ?  Otherwise
cirrrus will try to every day to rerun but take 1hr to time out, which is twice
as slow as the slowest OS.

I think this patch should be moved to the next CF and set to v16.

Thanks,
-- 
Justin

  52. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-31T09:33:18Z

    At Thu, 31 Mar 2022 00:37:07 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> On Thu, Mar 31, 2022 at 01:58:45PM +0900, Kyotaro Horiguchi wrote:
> > Thanks! Version 20 is attached.
> 
> The patch failed an all CI tasks, and seems to have caused the macos task to
> hang.
> 
> http://cfbot.cputube.org/kyotaro-horiguchi.html
> 
> Would you send a fixed patch, or remove this thread from the CFBOT ?  Otherwis
e
> cirrrus will try to every day to rerun but take 1hr to time out, which is twice
> as slow as the slowest OS.

That is found to be a thinko that causes mark files left behind in new
database created in the logged version of CREATE DATABASE. It is
easily fixed.

That being said, this failure revealed that pg_checksums or
pg_basebackup dislikes the mark files.  It happens even in a quite low
possibility. This would need further consideration and extra rounds of
reviews.

> I think this patch should be moved to the next CF and set to v16.

I don't think this can be commited to 15. So I post the fixed version
then move this to the next CF.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  53. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-31T09:36:34Z

    At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I don't think this can be commited to 15. So I post the fixed version
> then move this to the next CF.

Then done. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  54. Re: In-placre persistance change of a relation

    Jacob Champion <jchampion@timescale.com> — 2022-07-06T15:44:18Z

    On Thu, Mar 31, 2022 at 2:36 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > I don't think this can be commited to 15. So I post the fixed version
> > then move this to the next CF.
>
> Then done. Thanks!

Hello! This patchset will need to be rebased over latest -- looks like
b74e94dc27f (Rethink PROCSIGNAL_BARRIER_SMGRRELEASE) and 5c279a6d350
(Custom WAL Resource Managers) are interfering.

Thanks,
--Jacob

  55. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-07T08:24:59Z

    At Wed, 6 Jul 2022 08:44:18 -0700, Jacob Champion <jchampion@timescale.com> wrote in 
> On Thu, Mar 31, 2022 at 2:36 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Thu, 31 Mar 2022 18:33:18 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > > I don't think this can be commited to 15. So I post the fixed version
> > > then move this to the next CF.
> >
> > Then done. Thanks!
> 
> Hello! This patchset will need to be rebased over latest -- looks like
> b74e94dc27f (Rethink PROCSIGNAL_BARRIER_SMGRRELEASE) and 5c279a6d350
> (Custom WAL Resource Managers) are interfering.

Thank you for checking that!  It got a wider attack by b0a55e4329
(RelFileNumber). The commit message suggests "relfilenode" as files
should be replaced with "relation storage/file" so I did that in
ResetUnloggedRelationsInDbspaceDir.

This patch said that:

>		 * INIT forks are never loaded to shared buffer so no point in
>		 * dropping buffers for such files.

But actually some *buildempty() functions use ReadBufferExtended() for
INIT_FORK. So that's wrong. So, I did that but... I don't like that.
Or I don't like that some AMs leave buffers for INIT fork after. But I
feel I'm misunderstanding here since I don't understand how the INIT
fork can work as expected after a crash that happens before the next
checkpoint flushes the buffers.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  56. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-19T04:33:33Z

    (Mmm. I haven't noticed an annoying misspelling in the subejct X( )

> Thank you for checking that!  It got a wider attack by b0a55e4329
> (RelFileNumber). The commit message suggests "relfilenode" as files

Then, now I stepped on my own foot. Rebased also on nodefuncs
autogeneration.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  57. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-28T08:21:19Z

    Just rebased.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  58. Re: In-placre persistance change of a relation

    Ian Lawrence Barwick <barwick@gmail.com> — 2022-11-04T00:32:52Z

    2022年9月28日(水) 17:21 Kyotaro Horiguchi <horikyota.ntt@gmail.com>:
>
> Just rebased.

Hi

cfbot reports the patch no longer applies. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.

Thanks

Ian Barwick

  59. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-11-08T02:33:53Z

    At Fri, 4 Nov 2022 09:32:52 +0900, Ian Lawrence Barwick <barwick@gmail.com> wrote in 
> cfbot reports the patch no longer applies. As CommitFest 2022-11 is
> currently underway, this would be an excellent time to update the patch.

Indeed, thanks!  I'll do that in a few days.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  60. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-11-18T08:25:11Z

    At Tue, 08 Nov 2022 11:33:53 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Indeed, thanks!  I'll do that in a few days.

Got too late, but rebased.. The contents of the two patches in the
last version was a bit shuffled but they are fixed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  61. Re: In-placre persistance change of a relation

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-02-03T07:42:52Z

    I want to call out this part of this patch:

> Also this allows for the cleanup of files left behind in the crash of
> the transaction that created it.

This is interesting to a lot wider audience than ALTER TABLE SET 
LOGGED/UNLOGGED. It also adds most of the complexity, with the new 
marker files. Can you please split the first patch into two:

1. Cleanup of newly created relations on crash

2. ALTER TABLE SET LOGGED/UNLOGGED changes

Then we can review the first part independently.

Regarding the first part, I'm not sure the marker files are the best 
approach to implement it. You need to create an extra file for every 
relation, just to delete it at commit. It feels a bit silly, but maybe 
it's OK in practice. The undo log patch set solved this problem with the 
undo log, but it looks like that patch set isn't going anywhere. Maybe 
invent a very lightweight version of the undo log for this?

- Heikki

  62. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-02-07T04:47:59Z

    Thank you for the comment!

At Fri, 3 Feb 2023 08:42:52 +0100, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> I want to call out this part of this patch:
> 
> > Also this allows for the cleanup of files left behind in the crash of
> > the transaction that created it.
> 
> This is interesting to a lot wider audience than ALTER TABLE SET
> LOGGED/UNLOGGED. It also adds most of the complexity, with the new
> marker files. Can you please split the first patch into two:
> 
> 1. Cleanup of newly created relations on crash
> 
> 2. ALTER TABLE SET LOGGED/UNLOGGED changes
> 
> Then we can review the first part independently.

Ah, indeed.  I'll do that.

> Regarding the first part, I'm not sure the marker files are the best
> approach to implement it. You need to create an extra file for every
> relation, just to delete it at commit. It feels a bit silly, but maybe

Agreed. (But I didn't come up with better idea..)

> it's OK in practice. The undo log patch set solved this problem with
> the undo log, but it looks like that patch set isn't going
> anywhere. Maybe invent a very lightweight version of the undo log for
> this?

I didn't thought on that line. Yes, indeed the marker files are a kind
of undo log.

Anyway, I'll split the current patch to two parts as suggested.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  63. Re: In-placre persistance change of a relation

    Gregory Stark (as CFM) <stark.cfm@gmail.com> — 2023-03-01T19:56:25Z

    On Mon, 6 Feb 2023 at 23:48, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> Thank you for the comment!
>
> At Fri, 3 Feb 2023 08:42:52 +0100, Heikki Linnakangas <hlinnaka@iki.fi> wrote in
> > I want to call out this part of this patch:

Looks like this patch has received some solid feedback from Heikki and
you have a path forward. It's not currently building in the build farm
either.

I'll set the patch to Waiting on Author for now.


-- 
Gregory Stark
As Commitfest Manager

  64. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-03-03T09:03:53Z

    At Wed, 1 Mar 2023 14:56:25 -0500, "Gregory Stark (as CFM)" <stark.cfm@gmail.com> wrote in 
> On Mon, 6 Feb 2023 at 23:48, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >
> > Thank you for the comment!
> >
> > At Fri, 3 Feb 2023 08:42:52 +0100, Heikki Linnakangas <hlinnaka@iki.fi> wrote in
> > > I want to call out this part of this patch:
> 
> Looks like this patch has received some solid feedback from Heikki and
> you have a path forward. It's not currently building in the build farm
> either.
> 
> I'll set the patch to Waiting on Author for now.

Correctly they are three parts.

Correctly they are three parts. The attached patch is the first part -
the storage mark files, which are used to identify storage files that
have not been committed and should be removed during the next
startup. This feature resolves the issue of orphaned storage files
that may result from a crash occurring during the execution of a
transaction involving the creation of a new table.

I'll post all of the three parts shortly.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  65. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-03-17T06:16:34Z

    At Fri, 03 Mar 2023 18:03:53 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Correctly they are three parts. The attached patch is the first part -
> the storage mark files, which are used to identify storage files that
> have not been committed and should be removed during the next
> startup. This feature resolves the issue of orphaned storage files
> that may result from a crash occurring during the execution of a
> transaction involving the creation of a new table.
> 
> I'll post all of the three parts shortly.

Mmm. It took longer than I said, but this is the patch set that
includes all three parts.

1. "Mark files" to prevent orphan storage files for in-transaction
  created relations after a crash.

2. In-place persistence change: For ALTER TABLE SET LOGGED/UNLOGGED
  with wal_level minimal, and ALTER TABLE SET UNLOGGED with other
  wal_levels, the commands don't require a file copy for the relation
  storage. ALTER TABLE SET LOGGED with non-minimal wal_level emits
  bulk FPIs instead of a bunch of individual INSERTs.

3. An extension to ALTER TABLE SET (UN)LOGGED that can handle all
  tables in a tablespace at once.


As a side note, I quickly go over the behavior of the mark files
introduced by the first patch, particularly what happens when deletion
fails.

(1) The mark file for MAIN fork ("<oid>.u") corresponds to all forks,
    while the mark file for INIT fork ("<oid>_init.u") corresponds to
    INIT fork alone.

(2) The mark file is created just before the the corresponding storage
    file is made. This is always logged in the WAL.

(3) The mark file is deleted after removing the corresponding storage
    file during the commit and rollback. This action is logged in the
    WAL, too. If the deletion fails, an ERROR is output and the
    transaction aborts.

(4) If a crash leaves a mark file behind, server will try to delete it
    after successfully removing the corresponding storage file during
    the subsequent startup that runs a recovery. If deletion fails,
    server leaves the mark file alone with emitting a WARNING. (The
    same behavior for non-mark files.)

(5) If the deletion of the mark file fails, the leftover mark file
    prevents the creation of the corresponding storage file (causing
    an ERROR).  The leftover mark files don't result in the removal of
    the wrong files due to that behavior.

(6) The mark file for an INIT fork is created only when ALTER TABLE
    SET UNLOGGED is executed (not for CREATE UNLOGGED TABLE) to signal
    the crash-cleanup code to remove the INIT fork. (Otherwise the
    cleanup code removes the main fork instead. This is the main
    objective of introducing the mark files.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  66. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-04-25T07:54:57Z

    At Fri, 17 Mar 2023 15:16:34 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Mmm. It took longer than I said, but this is the patch set that
> includes all three parts.
> 
> 1. "Mark files" to prevent orphan storage files for in-transaction
>   created relations after a crash.
> 
> 2. In-place persistence change: For ALTER TABLE SET LOGGED/UNLOGGED
>   with wal_level minimal, and ALTER TABLE SET UNLOGGED with other
>   wal_levels, the commands don't require a file copy for the relation
>   storage. ALTER TABLE SET LOGGED with non-minimal wal_level emits
>   bulk FPIs instead of a bunch of individual INSERTs.
> 
> 3. An extension to ALTER TABLE SET (UN)LOGGED that can handle all
>   tables in a tablespace at once.
> 
> 
> As a side note, I quickly go over the behavior of the mark files
> introduced by the first patch, particularly what happens when deletion
> fails.
> 
> (1) The mark file for MAIN fork ("<oid>.u") corresponds to all forks,
>     while the mark file for INIT fork ("<oid>_init.u") corresponds to
>     INIT fork alone.
> 
> (2) The mark file is created just before the the corresponding storage
>     file is made. This is always logged in the WAL.
> 
> (3) The mark file is deleted after removing the corresponding storage
>     file during the commit and rollback. This action is logged in the
>     WAL, too. If the deletion fails, an ERROR is output and the
>     transaction aborts.
> 
> (4) If a crash leaves a mark file behind, server will try to delete it
>     after successfully removing the corresponding storage file during
>     the subsequent startup that runs a recovery. If deletion fails,
>     server leaves the mark file alone with emitting a WARNING. (The
>     same behavior for non-mark files.)
> 
> (5) If the deletion of the mark file fails, the leftover mark file
>     prevents the creation of the corresponding storage file (causing
>     an ERROR).  The leftover mark files don't result in the removal of
>     the wrong files due to that behavior.
> 
> (6) The mark file for an INIT fork is created only when ALTER TABLE
>     SET UNLOGGED is executed (not for CREATE UNLOGGED TABLE) to signal
>     the crash-cleanup code to remove the INIT fork. (Otherwise the
>     cleanup code removes the main fork instead. This is the main
>     objective of introducing the mark files.)

Rebased.

I fixed some code comments and commit messages. I fixed the wrong
arrangement of some changes among patches.  Most importantly, I fixed
the a bug based on a wrong assumption that init-fork is not resides on
shared buffers. Now smgrDoPendingCleanups drops buffer for a init-fork
to be removed.

The new fourth patch is a temporary fix for recently added code, which
will soon be no longer needed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  67. Re: In-placre persistance change of a relation

    Jakub Wartak <jakub.wartak@enterprisedb.com> — 2023-04-27T12:47:41Z

    On Tue, Apr 25, 2023 at 9:55 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Rebased.
>
> I fixed some code comments and commit messages. I fixed the wrong
> arrangement of some changes among patches.  Most importantly, I fixed
> the a bug based on a wrong assumption that init-fork is not resides on
> shared buffers. Now smgrDoPendingCleanups drops buffer for a init-fork
> to be removed.
>
> The new fourth patch is a temporary fix for recently added code, which
> will soon be no longer needed.
>

Hi Kyotaro,

I've retested v28 of the patch with everything that came to my mind
(basic tests, --enable-tap-tests, restarts/crashes along adding the
data, checking if there were any files left over and I've checked for
stuff that earlier was causing problems: GiST on geometry[PostGIS]).
The only thing I've not tested this time were the performance runs
done earlier. The patch passed all my very limited tests along with
make check-world. Patch looks good to me on the surface from a
usability point of view. I haven't looked at the code, so the patch
might still need an in-depth review.

Regards,
-Jakub Wartak.

  68. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-04-28T01:58:53Z

    (I find the misspelled subject makes it difficult to find the thread..)

At Thu, 27 Apr 2023 14:47:41 +0200, Jakub Wartak <jakub.wartak@enterprisedb.com> wrote in 
> On Tue, Apr 25, 2023 at 9:55 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Rebased.
> >
> > I fixed some code comments and commit messages. I fixed the wrong
> > arrangement of some changes among patches.  Most importantly, I fixed
> > the a bug based on a wrong assumption that init-fork is not resides on
> > shared buffers. Now smgrDoPendingCleanups drops buffer for a init-fork
> > to be removed.
> >
> > The new fourth patch is a temporary fix for recently added code, which
> > will soon be no longer needed.

This is no longer needed. Thank you, Thomas!

> Hi Kyotaro,
> 
> I've retested v28 of the patch with everything that came to my mind
> (basic tests, --enable-tap-tests, restarts/crashes along adding the
> data, checking if there were any files left over and I've checked for
> stuff that earlier was causing problems: GiST on geometry[PostGIS]).

Maybe it's fixed by dropping buffers.

> The only thing I've not tested this time were the performance runs
> done earlier. The patch passed all my very limited tests along with
> make check-world. Patch looks good to me on the surface from a
> usability point of view. I haven't looked at the code, so the patch
> might still need an in-depth review.

Thank you for conducting a thorough test. In this patchset, the first
one might be useful on its own and it is the most complex part. I'll
recheck it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  69. Re: In-placre persistance change of a relation

    Nathan Bossart <nathandbossart@gmail.com> — 2023-08-14T19:38:48Z

    I think there are some good ideas here.  I started to take a look at the
patches, and I've attached a rebased version of the patch set.  Apologies
if I am repeating any discussions from upthread.

First, I tested the time difference in ALTER TABLE SET UNLOGGED/LOGGED with
the patch applied, and the results looked pretty impressive.

	before:
	postgres=# alter table test set unlogged;
	ALTER TABLE
	Time: 5108.071 ms (00:05.108)
	postgres=# alter table test set logged;
	ALTER TABLE
	Time: 6747.648 ms (00:06.748)

	after:
	postgres=# alter table test set unlogged;
	ALTER TABLE
	Time: 25.609 ms
	postgres=# alter table test set logged;
	ALTER TABLE
	Time: 1241.800 ms (00:01.242)

My first question is whether 0001 is a prerequisite to 0002.  I'm assuming
it is, but the reason wasn't immediately obvious to me.  If it's just
nice-to-have, perhaps we could simplify the patch set a bit.  I see that
Heikki had some general concerns with the marker file approach [0], so
perhaps it is at least worth brainstorming some alternatives if we _do_
need it.

[0] https://postgr.es/m/9827ebd3-de2e-fd52-4091-a568387b1fc2%40iki.fi

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  70. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-08-24T02:22:32Z

    Thank you for looking this!

At Mon, 14 Aug 2023 12:38:48 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> I think there are some good ideas here.  I started to take a look at the
> patches, and I've attached a rebased version of the patch set.  Apologies
> if I am repeating any discussions from upthread.
> 
> First, I tested the time difference in ALTER TABLE SET UNLOGGED/LOGGED with
> the patch applied, and the results looked pretty impressive.
> 
> 	before:
> 	postgres=# alter table test set unlogged;
> 	ALTER TABLE
> 	Time: 5108.071 ms (00:05.108)
> 	postgres=# alter table test set logged;
> 	ALTER TABLE
> 	Time: 6747.648 ms (00:06.748)
> 
> 	after:
> 	postgres=# alter table test set unlogged;
> 	ALTER TABLE
> 	Time: 25.609 ms
> 	postgres=# alter table test set logged;
> 	ALTER TABLE
> 	Time: 1241.800 ms (00:01.242)

Thanks for confirmation. The difference between the both directions is
that making a table logged requires to emit WAL records for the entire
content.

> My first question is whether 0001 is a prerequisite to 0002.  I'm assuming
> it is, but the reason wasn't immediately obvious to me.  If it's just

In 0002, if a backend crashes after creating an init fork file but
before the associated commit, a lingering fork file could result in
data loss on the next startup. Thus, an utterly reliable file cleanup
mechanism is essential. 0001 also addresses the orphan storage files
issue arising from ALTER TABLE and similar commands.

> nice-to-have, perhaps we could simplify the patch set a bit.  I see that
> Heikki had some general concerns with the marker file approach [0], so
> perhaps it is at least worth brainstorming some alternatives if we _do_
> need it.

The rationale behind the file-based implementation is that any
leftover init fork file from a crash needs to be deleted before the
reinit(INIT) process kicks in, which happens irrelevantly to WAL,
before the start of crash recovery. I could implement it separately
from the reinit module, but I didn't since that results in almost a
duplication.

As commented in xlog.c, the purpose of the pre-recovery reinit CLEANUP
phase is to ensure hot standbys don't encounter erroneous unlogged
relations.  Based on that requirement, we need a mechanism to
guarantee that additional crucial operations are executed reliably at
the next startup post-crash, right before recovery kicks in (or reinit
CLEANUP). 0001 persists this data on a per-operation basis tightly
bonded to their target objects.

I could turn this into something like undo longs in a simple form, but
I'd rather not craft a general-purpose undo log system for this unelss
it's absolutely necessary.


> [0] https://postgr.es/m/9827ebd3-de2e-fd52-4091-a568387b1fc2%40iki.fi

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  71. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-09-04T08:37:48Z

    At Thu, 24 Aug 2023 11:22:32 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I could turn this into something like undo longs in a simple form, but
> I'd rather not craft a general-purpose undo log system for this unelss
> it's absolutely necessary.

This is a patch for a basic undo log implementation. It looks like it
works well for some orphan-files-after-a-crash and data-loss-on-reinit
cases.  However, it is far from complete and likely has issues with
crash-safety and the durability of undo log files (and memory leaks
and performance and..).

I'm posting this to move the discussion forward.

(This doesn't contain the third file "ALTER TABLE ..ALL IN TABLESPACE" part.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  72. Re: In-placre persistance change of a relation

    vignesh C <vignesh21@gmail.com> — 2024-01-09T09:37:20Z

    On Mon, 4 Sept 2023 at 16:59, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Thu, 24 Aug 2023 11:22:32 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> > I could turn this into something like undo longs in a simple form, but
> > I'd rather not craft a general-purpose undo log system for this unelss
> > it's absolutely necessary.
>
> This is a patch for a basic undo log implementation. It looks like it
> works well for some orphan-files-after-a-crash and data-loss-on-reinit
> cases.  However, it is far from complete and likely has issues with
> crash-safety and the durability of undo log files (and memory leaks
> and performance and..).
>
> I'm posting this to move the discussion forward.
>
> (This doesn't contain the third file "ALTER TABLE ..ALL IN TABLESPACE" part.)

CFBot shows compilation issues at [1] with:
09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_twophase.c.o: in function
`FinishPreparedTransaction':
[09:34:44.987] /tmp/cirrus-ci-build/build/../src/backend/access/transam/twophase.c:1569:
undefined reference to `AtEOXact_SimpleUndoLog'
[09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_xact.c.o: in function
`CommitTransaction':
[09:34:44.987] /tmp/cirrus-ci-build/build/../src/backend/access/transam/xact.c:2372:
undefined reference to `AtEOXact_SimpleUndoLog'
[09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_xact.c.o: in function
`AbortTransaction':
[09:34:44.987] /tmp/cirrus-ci-build/build/../src/backend/access/transam/xact.c:2878:
undefined reference to `AtEOXact_SimpleUndoLog'
[09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_xact.c.o: in function
`CommitSubTransaction':
[09:34:44.987] /tmp/cirrus-ci-build/build/../src/backend/access/transam/xact.c:5016:
undefined reference to `AtEOXact_SimpleUndoLog'
[09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_xact.c.o: in function
`AbortSubTransaction':
[09:34:44.987] /tmp/cirrus-ci-build/build/../src/backend/access/transam/xact.c:5197:
undefined reference to `AtEOXact_SimpleUndoLog'
[09:34:44.987] /usr/bin/ld:
src/backend/postgres_lib.a.p/access_transam_xact.c.o:/tmp/cirrus-ci-build/build/../src/backend/access/transam/xact.c:6080:
more undefined references to `AtEOXact_SimpleUndoLog' follow

[1] - https://cirrus-ci.com/task/5916232528953344

Regards,
Vignesh

  73. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-15T07:50:59Z

    At Tue, 9 Jan 2024 15:07:20 +0530, vignesh C <vignesh21@gmail.com> wrote in 
> CFBot shows compilation issues at [1] with:

Thanks!

The reason for those errors was that I didn't consider Meson at the
time. Additionally, the signature change of reindex_index() caused the
build failure. I fixed both issues. While addressing these issues, I
modified the simpleundolog module to honor
wal_sync_method. Previously, the sync method for undo logs was
determined independently, separate from xlog.c. However, I'm still not
satisfied with the method for handling PG_O_DIRECT.

In this version, I have added the changes to enable the use of
wal_sync_method outside of xlog.c as the first part of the patchset.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  74. Re: In-placre persistance change of a relation

    Peter Smith <smithpb2250@gmail.com> — 2024-01-22T04:36:31Z

    2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there was a CFbot test failure last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/3461/
[2] https://cirrus-ci.com/task/6050020441456640

Kind Regards,
Peter Smith.

  75. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-01-23T04:15:21Z

    At Mon, 22 Jan 2024 15:36:31 +1100, Peter Smith <smithpb2250@gmail.com> wrote in 
> 2024-01 Commitfest.
> 
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there was a CFbot test failure last time it was run [2]. Please have a
> look and post an updated version if necessary.

Thanks! I have added the necessary includes to the header file this
patch adds. With this change, "make headerscheck" now passes. However,
when I run "make cpluspluscheck" in my environment, it generates a
large number of errors in other areas, but I didn't find one related
to this patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  76. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-05-24T07:09:16Z

    Rebased.

Along with rebasing, I changed the interface of XLogFsyncFile() to
return a boolean instead of an error message.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  77. Re: In-placre persistance change of a relation

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-05-28T23:49:45Z

    On Fri, 24 May 2024 at 00:09, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> Along with rebasing, I changed the interface of XLogFsyncFile() to
> return a boolean instead of an error message.

Two notes after looking at this quickly during the advanced patch
feedback session:

1. I would maybe split 0003 into two separate patches. One to make SET
UNLOGGED fast, which seems quite easy to do because no WAL is needed.
And then a follow up to make SET LOGGED fast, which does all the
XLOG_FPI stuff.
2. When wal_level = minital, still some WAL logging is needed. The
pages that were changed since the last still need to be made available
for crash recovery.

  78. Re: In-placre persistance change of a relation

    Michael Paquier <michael@paquier.xyz> — 2024-06-04T00:09:12Z

    On Tue, May 28, 2024 at 04:49:45PM -0700, Jelte Fennema-Nio wrote:
> Two notes after looking at this quickly during the advanced patch
> feedback session:
> 
> 1. I would maybe split 0003 into two separate patches. One to make SET
> UNLOGGED fast, which seems quite easy to do because no WAL is needed.
> And then a follow up to make SET LOGGED fast, which does all the
> XLOG_FPI stuff.

Yeah, that would make sense.  The LOGGED->UNLOGGED part is
straight-forward because we only care about the init fork.  The
UNLOGGED->LOGGED case bugs me, though, a lot.

> 2. When wal_level = minitam, still some WAL logging is needed. The
> pages that were changed since the last still need to be made available
> for crash recovery.

More notes from me, as I was part of this session.

+         * XXXX: Some access methods don't support in-place persistence
+         * changes. GiST uses page LSNs to figure out whether a block has been
[...]
+        if (r->rd_rel->relkind == RELKIND_INDEX &&
+        /* GiST is excluded */
+            r->rd_rel->relam != BTREE_AM_OID &&
+            r->rd_rel->relam != HASH_AM_OID &&
+            r->rd_rel->relam != GIN_AM_OID &&
+            r->rd_rel->relam != SPGIST_AM_OID &&
+            r->rd_rel->relam != BRIN_AM_OID)

This knowledge should not be encapsulated in the backend code.  The
index AMs should be able to tell, instead, if they are able to support
this code path so as any out-of-core index AM can decide things on its
own.  This ought to be split in its own patch, simple enough as of a
boolean or a routine telling how this backend path should behave.

+            for (fork = 0; fork < INIT_FORKNUM; fork++)
+            {
+                if (smgrexists(RelationGetSmgr(r), fork))
+                    log_newpage_range(r, fork, 0,
+                                      smgrnblocks(RelationGetSmgr(r), fork),
+                                      false);
+            }

A simple copy of the blocks means that we keep anything bloated in
them, while a rewrite in ALTER TABLE means that we would start afresh
by deforming the tuples from the origin before giving them to the
target, without any bloat.  The compression of the FPWs and the
removal of the holes in the pages would surely limit the impact, but
this has not been discussed on this thread, and this is a nice
property of the existing implementation that would get silently
removed by this patch set.

Another point that Nathan has made is that it may be more appealling
to study how this is better than an integration with the multi-INSERT
APIs into AMs, so as it is possible to group the inserts in batches
rather than process them one-at-a-time, see [1].  I am ready to accept
that what this patch does is more efficient as long as everything is
block-based in some cases.  Still there is a risk-vs-gain argument
here, and I am not sure whether what we have here is a good tradeoff
compared to the potential risk of breaking things.  The amount of new
infrastructure is large for this code path.  Grouping the inserts in
large batches may finish by being more efficient than a WAL stream
full of FPWs, as well, even if toast values are deformed?  So perhaps
there is an argument for making that optional at query level, instead.
As a hole, I can say that grouping the INSERTs will be always more
efficient, while what we have here can be less efficient in some
cases.  I'm OK to be outvoted, but the level of complications created
by this block-based copy and WAL-logging concerns me when it comes to
tweaking the relpersistence like that.

[1]: https://commitfest.postgresql.org/48/4777/
--
Michael

  79. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-06-04T07:00:32Z

    Thank you for the comments.

# The most significant feedback I received was that this approach is
# not misdirected..

At Tue, 4 Jun 2024 09:09:12 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Tue, May 28, 2024 at 04:49:45PM -0700, Jelte Fennema-Nio wrote:
> > Two notes after looking at this quickly during the advanced patch
> > feedback session:
> > 
> > 1. I would maybe split 0003 into two separate patches. One to make SET
> > UNLOGGED fast, which seems quite easy to do because no WAL is needed.
> > And then a follow up to make SET LOGGED fast, which does all the
> > XLOG_FPI stuff.
> 
> Yeah, that would make sense.  The LOGGED->UNLOGGED part is
> straight-forward because we only care about the init fork.  The
> UNLOGGED->LOGGED case bugs me, though, a lot.

I indeed agree with that. Will do that in the next version.

> > 2. When wal_level = minitam, still some WAL logging is needed. The
> > pages that were changed since the last still need to be made available
> > for crash recovery.

I don't quite understand this.  It seems that you are reffering to the
LOGGED to UNLOGGED case. UNLOGGED tables are emptied after a crash,
and the newly created INIT fork does that trick. Maybe I'm
misunderstanding something, though.

> More notes from me, as I was part of this session.
> 
> +         * XXXX: Some access methods don't support in-place persistence
> +         * changes. GiST uses page LSNs to figure out whether a block has been
> [...]
> +        if (r->rd_rel->relkind == RELKIND_INDEX &&
> +        /* GiST is excluded */
> +            r->rd_rel->relam != BTREE_AM_OID &&
> +            r->rd_rel->relam != HASH_AM_OID &&
> +            r->rd_rel->relam != GIN_AM_OID &&
> +            r->rd_rel->relam != SPGIST_AM_OID &&
> +            r->rd_rel->relam != BRIN_AM_OID)
> 
> This knowledge should not be encapsulated in the backend code.  The
> index AMs should be able to tell, instead, if they are able to support
> this code path so as any out-of-core index AM can decide things on its
> own.  This ought to be split in its own patch, simple enough as of a
> boolean or a routine telling how this backend path should behave.

Right. I was hesitant to expand the scope before being certain that I
can proceed in this direction without significant objections. Now I
can include that in the next version.

> +            for (fork = 0; fork < INIT_FORKNUM; fork++)
> +            {
> +                if (smgrexists(RelationGetSmgr(r), fork))
> +                    log_newpage_range(r, fork, 0,
> +                                      smgrnblocks(RelationGetSmgr(r), fork),
> +                                      false);
> +            }
> 
> A simple copy of the blocks means that we keep anything bloated in
> them, while a rewrite in ALTER TABLE means that we would start afresh
> by deforming the tuples from the origin before giving them to the
> target, without any bloat.  The compression of the FPWs and the
> removal of the holes in the pages would surely limit the impact, but
> this has not been discussed on this thread, and this is a nice
> property of the existing implementation that would get silently
> removed by this patch set.

Sure. That bloat can be removed beforehand by explicitly running
VACUUM on the table if needed, but it would be ideal if the same
compression occurred automatically. Alternatively, it might be an
option to fall back to the existing path when the target table is
found to have excessive bloat (though I'm not sure how much should be
considered excessive). We could also allow users to decide by adding a
command option.

> Another point that Nathan has made is that it may be more appealling
> to study how this is better than an integration with the multi-INSERT
> APIs into AMs, so as it is possible to group the inserts in batches
> rather than process them one-at-a-time, see [1].  I am ready to accept
> that what this patch does is more efficient as long as everything is
> block-based in some cases.  Still there is a risk-vs-gain argument
> here, and I am not sure whether what we have here is a good tradeoff
> compared to the potential risk of breaking things.  The amount of new
> infrastructure is large for this code path.  Grouping the inserts in
> large batches may finish by being more efficient than a WAL stream
> full of FPWs, as well, even if toast values are deformed?  So perhaps
> there is an argument for making that optional at query level, instead.

I agree about the uncertainties. With the switching feature mentioned
above, it might be sufficient to use the multi-insert stuff in the
existing path. However, the uncertainties regarding performance would
still remain.

> As a hole, I can say that grouping the INSERTs will be always more
> efficient, while what we have here can be less efficient in some
> cases.  I'm OK to be outvoted, but the level of complications created
> by this block-based copy and WAL-logging concerns me when it comes to
> tweaking the relpersistence like that.

Of course, it is a promising option to move away from the
block-logging and fall back to the existing path using the
multi-insert stuff in the UNLOGGED to LOGGED case. Let me consider
that point.


Besides the above, even though this discussion might become
unnecessary, there was a concern that the blockwise logging might
result in unexpected outcomes due to unflushed buffer data. (although
I could be mistaken). I believe that is not the case because all
buffer blocks are flushed out beforehand.


> [1]: https://commitfest.postgresql.org/48/4777/

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  80. Re: In-placre persistance change of a relation

    Nathan Bossart <nathandbossart@gmail.com> — 2024-06-04T20:50:51Z

    +Bharath

On Tue, Jun 04, 2024 at 04:00:32PM +0900, Kyotaro Horiguchi wrote:
> At Tue, 4 Jun 2024 09:09:12 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
>> Another point that Nathan has made is that it may be more appealling
>> to study how this is better than an integration with the multi-INSERT
>> APIs into AMs, so as it is possible to group the inserts in batches
>> rather than process them one-at-a-time, see [1].  I am ready to accept
>> that what this patch does is more efficient as long as everything is
>> block-based in some cases.  Still there is a risk-vs-gain argument
>> here, and I am not sure whether what we have here is a good tradeoff
>> compared to the potential risk of breaking things.  The amount of new
>> infrastructure is large for this code path.  Grouping the inserts in
>> large batches may finish by being more efficient than a WAL stream
>> full of FPWs, as well, even if toast values are deformed?  So perhaps
>> there is an argument for making that optional at query level, instead.
> 
> I agree about the uncertainties. With the switching feature mentioned
> above, it might be sufficient to use the multi-insert stuff in the
> existing path. However, the uncertainties regarding performance would
> still remain.

Bharath, does the multi-INSERT stuff apply when changing a table to be
LOGGED?  If so, I think it would be interesting to compare it with the FPI
approach being discussed here.

-- 
nathan

  81. Re: In-placre persistance change of a relation

    Michael Paquier <michael@paquier.xyz> — 2024-06-05T04:52:21Z

    On Tue, Jun 04, 2024 at 03:50:51PM -0500, Nathan Bossart wrote:
> Bharath, does the multi-INSERT stuff apply when changing a table to be
> LOGGED?  If so, I think it would be interesting to compare it with the FPI
> approach being discussed here.

The answer to this question is yes AFAIK.  Look at patch 0002 in the
latest series posted here, that touches ATRewriteTable() in
tablecmds.c where the rewrite happens should a relation's
relpersistence, AM, column or default requires a switch (particularly
if more than one property is changed in a single command, grep for
AT_REWRITE_*):
https://www.postgresql.org/message-id/CALj2ACUz5+_YNEa4ZY-XG960_oXefM50MjD71VgSCAVDkF3bzQ@mail.gmail.com

I've just read through the patch set, and they are rather pleasant to
the eye.  I have comments about them, actually, but that's a topic for
the other thread.
--
Michael

  82. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-08-31T16:09:25Z

    Hello.

It's been a while. Based on our previous face-to-face discussions, I
have been restructuring the patch set. During this process, I found
several missing parts and issues, which led to almost everything being
rewritten. However, I believe the updates are now better organized and
more understandable.

The current patch set broadly consists of the following elements:

- Core feature: Switching buffer persistence (0007) remains mostly the
  same as before, but the creation and deletion of INIT fork files
  have undergone significant modifications. Part of this functionality
  has been moved to commit records.

- UNDO log(0002): This handles file deletion during transaction aborts,
  which was previously managed, in part, by the commit XLOG record at
  the end of a transaction.

- Prevent orphan files after a crash (0005): This is another use-case
  of the UNDO log system.

- Extension of smgr (0012), pendingDeletes (0014), and commit XLOG
  records (0013): These have been extended to handle file deletion at
  the fork level instead of the relfilenumber level. While this
  extension applies to both commit and abort operations, only the file
  deletion process for aborts has been moved to the UNDO log. As a
  result, file deletions during commits continue to be managed by
  commit records.

Here are some issues. Depending on how these points are addressed,
this patch set might be dropped. (Or, this patch might already be too
large for its intended effect.)

- Consecutive changes to the persistence of the same table within a
  single transaction are prohibited (0007). Allowing this would
  complicate pendingDeletes and a similar mechanism added to
  bufmgr. Also, due to the append-only nature of the UNDO log, the
  entire process, including subtransaction handling, could not be made
  consistent easily.

- PREPARE is prohibited for transactions that have altered table
  persistence(0009). This is because I haven't found a simple way to
  ensure consistent switching of buffer persistence if the server
  crashes after PREPARE and then commits the transaction after
  recovery.

- Data updates within a single transaction after changing the table's
  persistence are also prohibited(0008). This restriction is necessary
  because if an index update triggers page splits after changing the
  persistence to UNLOGGED, WAL might become inapplicable.

The last point, in particular, has a significant impact on usability,
but it seems to be fundamentally unavoidable. Since heap updates
appear to be fine, one possible approach could be to give up on
in-place persistence changes for indexes.

Regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  83. Re: In-placre persistance change of a relation

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-09-01T19:15:00Z

    On 31/08/2024 19:09, Kyotaro Horiguchi wrote:
> - UNDO log(0002): This handles file deletion during transaction aborts,
>    which was previously managed, in part, by the commit XLOG record at
>    the end of a transaction.
> 
> - Prevent orphan files after a crash (0005): This is another use-case
>    of the UNDO log system.

Nice, I'm very excited if we can fix that long-standing issue! I'll try 
to review this properly later, but at a quick 5 minute glance, one thing 
caught my eye:

This requires fsync()ing the per-xid undo log file every time a relation 
is created. I fear that can be a pretty big performance hit for 
workloads that repeatedly create and drop small tables. Especially if 
they're otherwise running with synchronous_commit=off. Instead of 
flushing the undo log file after every write, I'd suggest WAL-logging 
the undo log like regular relations and SLRUs. So before writing the 
entry to the undo log, WAL-log it. And with a little more effort, you 
could postpone creating the files altogether until a checkpoint happens, 
similar to how twophase state files are checkpointed nowadays.

I wonder if the twophase state files and undo log files should be merged 
into one file. They're similar in many ways: there's one file per 
transaction, named using the XID. I haven't thought this fully through, 
just a thought..

> +static void
> +undolog_set_filename(char *buf, TransactionId xid)
> +{
> +	snprintf(buf, MAXPGPATH, "%s/%08x", SIMPLE_UNDOLOG_DIR, xid);
> +}

I'd suggest using FullTransactionId. Doesn't matter much, but seems like 
a good future-proofing.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  84. Re: In-placre persistance change of a relation

    Michael Paquier <michael@paquier.xyz> — 2024-09-02T00:30:20Z

    On Sun, Sep 01, 2024 at 10:15:00PM +0300, Heikki Linnakangas wrote:
> I wonder if the twophase state files and undo log files should be merged
> into one file. They're similar in many ways: there's one file per
> transaction, named using the XID. I haven't thought this fully through, just
> a thought..

Hmm.  It could be possible to extract some of this knowledge out of
twophase.c and design some APIs that could be used for both, but would
that be really necessary?  The 2PC data and the LSNs used by the files
to check if things are replayed or on disk rely on
GlobalTransactionData that has its own idea of things and timings at
recovery.

Or perhaps your point is actually to do that and add one layer for the
file handlings and their flush timings?  I am not sure, TBH, what this
thread is trying to fix is complicated enough that it may be better to
live with two different code paths.  But perhaps my gut feeling is
just wrong reading your paragraph.
--
Michael

  85. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-05T06:34:03Z

    Hello.

Thank you for the response.

At Sun, 1 Sep 2024 22:15:00 +0300, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> On 31/08/2024 19:09, Kyotaro Horiguchi wrote:
> > - UNDO log(0002): This handles file deletion during transaction aborts,
> >    which was previously managed, in part, by the commit XLOG record at
> >    the end of a transaction.
> > - Prevent orphan files after a crash (0005): This is another use-case
> >    of the UNDO log system.
> 
> Nice, I'm very excited if we can fix that long-standing issue! I'll
> try to review this properly later, but at a quick 5 minute glance, one
> thing caught my eye:
> 
> This requires fsync()ing the per-xid undo log file every time a
> relation is created. I fear that can be a pretty big performance hit
> for workloads that repeatedly create and drop small tables. Especially

I initially thought that one additional file manipulation during file
creation wouldn't be an issue. However, the created storage file isn't
being synced, so your concern seems valid.

> if they're otherwise running with synchronous_commit=off. Instead of
> flushing the undo log file after every write, I'd suggest WAL-logging
> the undo log like regular relations and SLRUs. So before writing the
> entry to the undo log, WAL-log it. And with a little more effort, you
> could postpone creating the files altogether until a checkpoint
> happens, similar to how twophase state files are checkpointed
> nowadays.

I thought that an UNDO log file not flushed before the last checkpoint
might not survive a system crash. However, including UNDO files in the
checkpointing process resolves that concern. Thansk you for the
suggestion.

> I wonder if the twophase state files and undo log files should be
> merged into one file. They're similar in many ways: there's one file
> per transaction, named using the XID. I haven't thought this fully
> through, just a thought..

Precisely, UNDO log files are created per subtransaction, unlike
twophase files. It might be possible if we allow the twophase files
(as they are currently named) to be overwritten or modified at every
subcommit. If ULOG contents are WAL-logged, these two things will
become even more similar. However, I'm not planning to include that in
the next version for now.

> > +static void
> > +undolog_set_filename(char *buf, TransactionId xid)
> > +{
> > +	snprintf(buf, MAXPGPATH, "%s/%08x", SIMPLE_UNDOLOG_DIR, xid);
> > +}
> 
> I'd suggest using FullTransactionId. Doesn't matter much, but seems
> like a good future-proofing.

Agreed. Will fix it in the next vesion.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  86. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-09-05T06:46:29Z

    At Mon, 2 Sep 2024 09:30:20 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Sun, Sep 01, 2024 at 10:15:00PM +0300, Heikki Linnakangas wrote:
> > I wonder if the twophase state files and undo log files should be merged
> > into one file. They're similar in many ways: there's one file per
> > transaction, named using the XID. I haven't thought this fully through, just
> > a thought..
> 
> Hmm.  It could be possible to extract some of this knowledge out of
> twophase.c and design some APIs that could be used for both, but would
> that be really necessary?  The 2PC data and the LSNs used by the files
> to check if things are replayed or on disk rely on
> GlobalTransactionData that has its own idea of things and timings at
> recovery.

I'm not sure, but I feel that Heikki mentioned only about using the
file format and in/out functions if the file formats of the two are
sufficiently overlapping.

> Or perhaps your point is actually to do that and add one layer for the
> file handlings and their flush timings?  I am not sure, TBH, what this
> thread is trying to fix is complicated enough that it may be better to
> live with two different code paths.  But perhaps my gut feeling is
> just wrong reading your paragraph.

I believe this statement is valid, so I’m not in a hurry to do this.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  87. Re: In-placre persistance change of a relation

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-10-28T13:33:41Z

    On 31/08/2024 19:09, Kyotaro Horiguchi wrote:
> Subject: [PATCH v34 03/16] Remove function for retaining files on outer
>  transaction aborts
> 
> The function RelationPreserveStorage() was initially created to keep
> storage files committed in a subtransaction on the abort of outer
> transactions. It was introduced by commit b9b8831ad6 in 2010, but no
> use case for this behavior has emerged since then. If we move the
> at-commit removal feature of storage files from pendingDeletes to the
> UNDO log system, the UNDO system would need to accept the cancellation
> of already logged entries, which makes the system overly complex with
> no benefit. Therefore, remove the feature.

I don't think that's quite right. I don't think this was meant for 
subtransaction aborts, but to make sure that if the top-transaction 
aborts after AtEOXact_RelationMap() has already been called, we don't 
remove the new relation. AtEOXact_RelationMap() is called very late in 
the commit process to keep the window as small as possible, but if it 
nevertheless happens, the consequences are pretty bad if you remove a 
relation file that is in fact needed.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  88. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-10-31T08:01:30Z

    At Sun, 1 Sep 2024 22:15:00 +0300, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> On 31/08/2024 19:09, Kyotaro Horiguchi wrote:
> This requires fsync()ing the per-xid undo log file every time a
> relation is created. I fear that can be a pretty big performance hit
> for workloads that repeatedly create and drop small tables. Especially
> if they're otherwise running with synchronous_commit=off. Instead of
> flushing the undo log file after every write, I'd suggest WAL-logging
> the undo log like regular relations and SLRUs. So before writing the
> entry to the undo log, WAL-log it. And with a little more effort, you
> could postpone creating the files altogether until a checkpoint
> happens, similar to how twophase state files are checkpointed
> nowadays.

After some delays, here’s the new version. In this update, UNDO logs
are WAL-logged and processed in memory under most conditions. During
checkpoints, they’re flushed to files, which are then read when a
specific XID’s UNDO log is accessed for the first time during
recovery.

The biggest changes are in patches 0001 through 0004 (equivalent to
the previous 0001-0002). After that, there aren’t any major
changes. Since this update involves removing some existing features,
I’ve split these parts into multiple smaller identity transformations
to make them clearer.

As for changes beyond that, the main one is lifting the previous
restriction on PREPARE for transactions after a persistence
change. This was made possible because, with the shift to in-memory
processing of UNDO logs, commit-time crash recovery detection is now
simpler. Additional changes include completely removing the
abort-handling portion from the pendingDeletes mechanism (0008-0010).


> I'd suggest using FullTransactionId. Doesn't matter much, but seems
> like a good future-proofing.

And, the patchset now uses full transaction ids.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  89. Re: In-placre persistance change of a relation

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-10-31T21:24:36Z

    On 31/10/2024 10:01, Kyotaro Horiguchi wrote:
> After some delays, here’s the new version. In this update, UNDO logs
> are WAL-logged and processed in memory under most conditions. During
> checkpoints, they’re flushed to files, which are then read when a
> specific XID’s UNDO log is accessed for the first time during
> recovery.
> 
> The biggest changes are in patches 0001 through 0004 (equivalent to
> the previous 0001-0002). After that, there aren’t any major
> changes. Since this update involves removing some existing features,
> I’ve split these parts into multiple smaller identity transformations
> to make them clearer.
> 
> As for changes beyond that, the main one is lifting the previous
> restriction on PREPARE for transactions after a persistence
> change. This was made possible because, with the shift to in-memory
> processing of UNDO logs, commit-time crash recovery detection is now
> simpler. Additional changes include completely removing the
> abort-handling portion from the pendingDeletes mechanism (0008-0010).

In this patch version, the undo log is kept in dynamic shared memory. It 
can grow indefinitely. On a checkpoint, it's flushed to disk.

If I'm reading it correctly, the undo records are kept in the DSA area 
even after it's flushed to disk. That's not necessary; system never 
needs to read the undo log unless there's a crash, so there's no need to 
keep it in memory after it's been flushed to disk. That's true today; we 
could start relying on the undo log to clean up on abort even when 
there's no crash, but I think it's a good design to not do that and rely 
on backend-private state for non-crash transaction abort.


I'd suggest doing this the other way 'round. Let's treat the on-disk 
representation as the primary representation, not the in-memory one. 
Let's use a small fixed-size shared memory area just as a write buffer 
to hold the dirty undo log entries that haven't been written to disk 
yet. Most transactions are short, so most undo log entries never need to 
be flushed to disk, but I think it'll be simpler to think of it that 
way. On checkpoint, flush all the buffered dirty entries from memory to 
disk and clear the buffer. Also do that if the buffer fills up.

A high-level overview comment of the on-disk format would be nice. If I 
understand correctly, there's a magic constant at the beginning of each 
undo file, followed by UndoLogRecords. There are no other file headers 
and no page structure within the file.

That format seems reasonable. For cross-checking, maybe add the XID to 
the file header too. There is a separate CRC value on each record, which 
is nice, but not strictly necessary since the writes to the UNDO log are 
WAL-logged. The WAL needs CRCs on each record to detect the end of log, 
but the UNDO log doesn't need that. Anyway, it's fine.


I somehow dislike the file per subxid design. I'm sure it works, it's 
just more of a feeling that it doesn't feel right. I'm somewhat worried 
about ending up with lots of files, if you e.g. use temporary tables 
with subtransactions heavily. Could we have just one file per top-level 
XID? I guess that can become a problem too, if you have a lot of aborted 
subtransactions. The UNDO records for the aborted subtransactions would 
bloat the undo file. But maybe that's nevertheless better?

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  90. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-11-05T04:25:26Z

    Thank you for the quick comments.

At Thu, 31 Oct 2024 23:24:36 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> On 31/10/2024 10:01, Kyotaro Horiguchi wrote:
> > After some delays, here’s the new version. In this update, UNDO logs
> > are WAL-logged and processed in memory under most conditions. During
> > checkpoints, they’re flushed to files, which are then read when a
> > specific XID’s UNDO log is accessed for the first time during
> > recovery.
> > The biggest changes are in patches 0001 through 0004 (equivalent to
> > the previous 0001-0002). After that, there aren’t any major
> > changes. Since this update involves removing some existing features,
> > I’ve split these parts into multiple smaller identity transformations
> > to make them clearer.
> > As for changes beyond that, the main one is lifting the previous
> > restriction on PREPARE for transactions after a persistence
> > change. This was made possible because, with the shift to in-memory
> > processing of UNDO logs, commit-time crash recovery detection is now
> > simpler. Additional changes include completely removing the
> > abort-handling portion from the pendingDeletes mechanism (0008-0010).
> 
> In this patch version, the undo log is kept in dynamic shared
> memory. It can grow indefinitely. On a checkpoint, it's flushed to
> disk.
> 
> If I'm reading it correctly, the undo records are kept in the DSA area
> even after it's flushed to disk. That's not necessary; system never
> needs to read the undo log unless there's a crash, so there's no need

The system also needs to read the undo log whenever additional undo
logs are added. In this version, I’ve moved all abort-time
pendingDeletes data entirely to the undo logs. In other words, the DSA
area is expanded in exchange for reducing the pendingDelete list. As a
result, there is minimal impact on overall memory usage. Additionally,
the current flushing code is straightforward because it relies on the
in-memory primary image. If we drop the in-memory image during flush,
we might need exclusive locking or possibly some ordering
techniques. Anyway, I’ll consider that approach.

> to keep it in memory after it's been flushed to disk. That's true
> today; we could start relying on the undo log to clean up on abort
> even when there's no crash, but I think it's a good design to not do
> that and rely on backend-private state for non-crash transaction
> abort.

Hmm. Sounds reasonable. In the next version, I'll revert the changes
to pendingDeletes and adjust it to just discard the log on regular
aborts.

> I'd suggest doing this the other way 'round. Let's treat the on-disk
> representation as the primary representation, not the in-memory
> one. Let's use a small fixed-size shared memory area just as a write
> buffer to hold the dirty undo log entries that haven't been written to
> disk yet. Most transactions are short, so most undo log entries never
> need to be flushed to disk, but I think it'll be simpler to think of
> it that way. On checkpoint, flush all the buffered dirty entries from
> memory to disk and clear the buffer. Also do that if the buffer fills
> up.

I'd like to clarify the specific concept of these fixed-length memory
slots. Is it something like this: each slot is keyed by an XID,
followed by an in-file offset and a series of, say, 1024-byte areas?
When writing a log for a new XID, if no slot is available, the backend
would immediately evict the slot with the smallest XID to disk to free
up space. If an existing slot runs out of space while writing new
logs, the backend would flush it immediately and continue using the
area. Is this correct? Additionally, if multiple processes try to
write to a single slot, stricter locking might be needed. For example,
if a slot is evicted by a backend other than its user, exclusive
control might be required during the file write.  jjjIs there any
effective way to avoid such locking? In the current patch set, I’m
avoiding any impact on the backend from checkpointer file writes by
treating the in-memory image as primary. And regarding the number of
these areas… although I’m not entirely sure, it seems unlikely we’d
have hundreds of sessions simultaneously creating tables, so would it
make sense to make this configurable, with a default of around 32
areas?

> A high-level overview comment of the on-disk format would be nice. If
> I understand correctly, there's a magic constant at the beginning of
> each undo file, followed by UndoLogRecords. There are no other file
> headers and no page structure within the file.

Right.

> That format seems reasonable. For cross-checking, maybe add the XID to
> the file header too. There is a separate CRC value on each record,
> which is nice, but not strictly necessary since the writes to the UNDO
> log are WAL-logged. The WAL needs CRCs on each record to detect the
> end of log, but the UNDO log doesn't need that. Anyway, it's fine.

For the first point, I considered it when designing the previous patch
set but chose not to implement it. As for the CRC, you're right - it’s
simply a leftover from the previous design. I have no issues with
following both points.

> I somehow dislike the file per subxid design. I'm sure it works, it's
> just more of a feeling that it doesn't feel right. I'm somewhat
> worried about ending up with lots of files, if you e.g. use temporary
> tables with subtransactions heavily. Could we have just one file per

I first thought the same thing when working on the previos patch.

> top-level XID? I guess that can become a problem too, if you have a
> lot of aborted subtransactions. The UNDO records for the aborted
> subtransactions would bloat the undo file. But maybe that's
> nevertheless better?

In the current patch set, normal abort processing is handled by the
UNDO log, so maintaining the performance of the UNDO process is
essential. If we were to return this to pendingDeletes, it might also
be feasible to add an XID cancellation record to the UNDO log and scan
the entire file once before executing individual logs. I’ll give it
some thought.


At Mon, 28 Oct 2024 15:33:41 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> On 31/08/2024 19:09, Kyotaro Horiguchi wrote:
> > Subject: [PATCH v34 03/16] Remove function for retaining files on
> > outer
> >  transaction aborts
> > The function RelationPreserveStorage() was initially created to keep
> > storage files committed in a subtransaction on the abort of outer
> > transactions. It was introduced by commit b9b8831ad6 in 2010, but no
> > use case for this behavior has emerged since then. If we move the
> > at-commit removal feature of storage files from pendingDeletes to the
> > UNDO log system, the UNDO system would need to accept the cancellation
> > of already logged entries, which makes the system overly complex with
> > no benefit. Therefore, remove the feature.
> 
> I don't think that's quite right. I don't think this was meant for
> subtransaction aborts, but to make sure that if the top-transaction
> aborts after AtEOXact_RelationMap() has already been called, we don't
> remove the new relation. AtEOXact_RelationMap() is called very late in

Hmm. I believe I wrote that. It prevents storage removal once it’s
committed in any subtransaction, even if that subtransaction is
finally aborted, including by the top transaction.

> the commit process to keep the window as small as possible, but if it
> nevertheless happens, the consequences are pretty bad if you remove a
> relation file that is in fact needed.

However, on second thought, it does seem odd. I may have confused
something here. If pendingDeletes is restored and undo cancellation is
implemented, this change would be unnecessary.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  91. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-11-11T01:25:47Z

    A bit out of the blue, but I remembered the reason why I could make
that change I previously agreed seemed off. Just thought I’d let you
know.

At Tue, 05 Nov 2024 13:25:26 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
me> > the commit process to keep the window as small as possible, but if it
me> > nevertheless happens, the consequences are pretty bad if you remove a
me> > relation file that is in fact needed.
me> 
me> However, on second thought, it does seem odd. I may have confused
me> something here. If pendingDeletes is restored and undo cancellation is
me> implemented, this change would be unnecessary.

The change would indeed be incorrect if updates to mapped relations
could occur within subtransactions. However, in reality, trying to
perform such an operation raises an error (something like “cannot do
this in a subtransaction”) and is rejected. So, there’s actually no
path where the removed code would be used. That’s why I judged it was
safe to remove that part. However, from that perspective, I think the
explanations in the comments and commit messages were somewhat lacking
or missed the point.

Currently, I’m leaning toward implementing per-relation undo
cancellation. Previously, this path was active even during normal
aborts, so there were performance concerns, but now it only runs
during recovery cleanup, so there are no performance issues with
handling cancellation. In the current state, the code has been
simplified overall.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  92. Re: In-placre persistance change of a relation

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-12-27T08:25:02Z

    Hello. This is the updated version.

(Sorry for the delay; I've been a little swamped.)

- Undo logs are primarily stored in a fixed number of fixed-length
  slots and are spilled into files under some conditions.

  The number of slots is 32 (ULOG_SLOT_NUM), and the buffer length is
  1024 (ULOG_SLOT_BUF_LEN). Both are currently non-configurable.

- Undo logs are now used only during recovery and no longer involved
  in transaction ends for normal backends. Pending deletes for aborts
  have been restored.

- Undo logs are stored on a per-Top-XID basis.

- RelationPreserverStorate() is no longer modified.

In this version, in the part following the introduction of orphan
storage prevention, the restriction on prepared transactions
persisting beyond server crashes (i.e., the prohibition) has been
removed. This is because handling for such cases has been reverted to
pendingDeletes.

Let me know if you have any questions or concerns.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  93. Re: In-placre persistance change of a relation

    Thom Brown <thom@linux.com> — 2025-04-04T21:29:03Z

    On Fri, 27 Dec 2024 at 08:26, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> Hello. This is the updated version.
>
> (Sorry for the delay; I've been a little swamped.)
>
> - Undo logs are primarily stored in a fixed number of fixed-length
>   slots and are spilled into files under some conditions.
>
>   The number of slots is 32 (ULOG_SLOT_NUM), and the buffer length is
>   1024 (ULOG_SLOT_BUF_LEN). Both are currently non-configurable.
>
> - Undo logs are now used only during recovery and no longer involved
>   in transaction ends for normal backends. Pending deletes for aborts
>   have been restored.
>
> - Undo logs are stored on a per-Top-XID basis.
>
> - RelationPreserverStorate() is no longer modified.
>
> In this version, in the part following the introduction of orphan
> storage prevention, the restriction on prepared transactions
> persisting beyond server crashes (i.e., the prohibition) has been
> removed. This is because handling for such cases has been reverted to
> pendingDeletes.
>
> Let me know if you have any questions or concerns.

I just went to give this a test drive, but HEAD has drifted too far,
at least for 0017 to apply. Could you please rebase and make the
necessary modifications?

Thanks

Thom

  94. Re: In-placre persistance change of a relation

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-04-05T20:17:50Z

    On 05/04/2025 00:29, Thom Brown wrote:
> On Fri, 27 Dec 2024 at 08:26, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>>
>> Hello. This is the updated version.
>>
>> (Sorry for the delay; I've been a little swamped.)
>>
>> - Undo logs are primarily stored in a fixed number of fixed-length
>>    slots and are spilled into files under some conditions.
>>
>>    The number of slots is 32 (ULOG_SLOT_NUM), and the buffer length is
>>    1024 (ULOG_SLOT_BUF_LEN). Both are currently non-configurable.
>>
>> - Undo logs are now used only during recovery and no longer involved
>>    in transaction ends for normal backends. Pending deletes for aborts
>>    have been restored.
>>
>> - Undo logs are stored on a per-Top-XID basis.
>>
>> - RelationPreserverStorate() is no longer modified.
>>
>> In this version, in the part following the introduction of orphan
>> storage prevention, the restriction on prepared transactions
>> persisting beyond server crashes (i.e., the prohibition) has been
>> removed. This is because handling for such cases has been reverted to
>> pendingDeletes.
>>
>> Let me know if you have any questions or concerns.
> 
> I just went to give this a test drive, but HEAD has drifted too far,
> at least for 0017 to apply. Could you please rebase and make the
> necessary modifications?

I had a quick look a this latest version now, up to 
"v36-0005-Prevent-orphan-storage-files-after-server-crash.patch" 
(because I'm very interested in that, but not in the rest of the 
patches). Sorry I haven't gotten around to it earlier.

Overall I'm pretty happy with the design. The main thing that's now 
missing is documentation. The main SGML docs should surely have a 
section on the UNDO log. A new README to describe how modules should use 
the undo log etc. would probably also be in order.

Off the top of my head, some subtle high-level things that should be 
explained somewhere:

- The UNDO log is only used to clean up after crash of a relation 
creation. It is *not* used for aborting or crash recovery of data, like 
on most systems. As a result, it's not as performance critical as you 
might think.

- The UNDO log is not a single sequential log like on many other 
systems. One way to think about it is that it's a per-transaction file, 
with a cache in shared memory for performance.

- The UNDO log is not used to handle controlled aborts, only for cleanup 
after a crash.

- What happens if you fail to process the UNDO log for some reason? Some 
storage files are leaked. Is that still considered OK, i.e. is the UNDO 
log a nice-to-have, or are there some more serious consequences?

- The interaction between REDO and UNDO. Every record inserted to the 
UNDO log of a transaction is WAL-logged in the REDO log. The undo log is 
like data file in that sense. Writing to the undo log follows the usual 
"WAL-before-write" rule: the WAL is flushed before the corresponding 
undo log entry is written to disk. (Is that true? I'm not 100% sure)

- When a new relation is created, do you flush the WAL before creating 
the file? Or is there still a small window where it can leak, if the 
file creation makes it to disk before crash but the undo log (or the WAL 
record of the undo log entry) does not?

Have you done any performance testing of this? By "this" I mean the 
overhead of the undo-logging on create/drop table.

-- 
Heikki Linnakangas
Neon (https://neon.tech)