Update last written LSN for gin/gist index metadata #182

knizhnik · 2022-07-14T21:11:32Z

This commit fix the problem with pg_regress with last_written_lsn_cache.
Indexes which are populated in special way (gin,gist,spgist,...) requires update of last written LSN for relation metadata

aome510

For some reason, I didn't receive notification for review requested in this PR. LGTM, but let's wait for CIs in https://github.com/neondatabase/neon/tree/exp-07-18 to make sure tests with the new changes passed.

hlinnaka · 2022-07-18T15:53:47Z

src/backend/access/gin/gininsert.c

 		SetLastWrittenLSN(XactLastRecEnd, index->rd_smgr->smgr_rnode.node.relNode, 0, RelationGetNumberOfBlocks(index));
+		SetLastWrittenLSN(XactLastRecEnd, index->rd_smgr->smgr_rnode.node.relNode, REL_METADATA_PSEUDO_BLOCKNO, REL_METADATA_PSEUDO_BLOCKNO);


This API where you have to call SetLastWrittenLSN twice, is a bit awkward. In all of these calls where you pass REL_METADATA_PSEUDO_BLOCKNO, you have just loaded the relation with data. How replacing SetLastWrittenLSN() with two functions:

SetLastWrittenLSNForRelation(Relation, ForkNumber, XLogRecPtr); SetLastWrittenLSNForBlock(RelFileNode, ForkNumber, BlockNumber, XLogRecPtr);

hlinnaka · 2022-07-18T15:54:23Z

but let's wait for CIs in https://github.com/neondatabase/neon/tree/exp-07-18 to make sure tests with the new changes passed.

thanks!

…ions

hlinnaka · 2022-07-19T04:45:04Z

contrib/neon/pagestore_smgr.c

-/*
- * Pseudo block number used to associate LSN with relation metadata (relation size)
- */
-#define REL_METADATA_PSEUDO_BLOCKNO InvalidBlockNumber
-


You can move this back here

Unfortunately - not, it is use in pagestore_smgr.c in zenith_get_request_lsn -> GeLastWrittenLsn.
Certainly it is possible to add two versions of GetLastWrittenLsn function and somehow explain zenith_get_request_lsn which version to call. But it complicates code and I am not sure that it makes it ore readable. So I left it as it is.

hlinnaka · 2022-07-19T07:32:49Z

Got a CI failure with this:

test_runner/batch_others/test_recovery.py:64: in test_pageserver_recovery
    cur.execute("select count(*) from foo")
E   psycopg2.errors.IoError: could not read block 1 in rel 1663/12972/2663.0 from page server at lsn 0/016AE7D8
E   LINE 1: select count(*) from foo
E                                ^
E   DETAIL:  page server returned error: initdb failed

hlinnaka · 2022-07-19T07:34:44Z

It looks like this isn't really ready yet. How about we revert these changes, ie. commit 595ac69 and 7517d1c, so that we can ship the other changes on the 'main' branch. Namely, adding uuid-ossp to the list of supported extensions.

knizhnik · 2022-07-19T15:07:31Z

I can not reproduce the problem with test_recovery failure locally, did you? Or it just happens in CI?

Certainly, if there are such errors and this patch is blocking other patches, then it should be undone.
Do you want me to do it? Just reset main branch to 9c99008
and then apply db67809 on top of it?

Also I have implemented version of last written LSN cache which uses buffer tag as a key:
https://github.com/neondatabase/postgres/tree/last_written_lsn_buftag_cache
I still think that it is enough to have relnode as a key, because it makes comparison/hash calculation operations several times faster. And probability of collision is small. But if you think that it is more important to avoid conflicts we can use this branch.

So, if we undo this commits, what do you think will be the next steps with this PR?
Should we try to fix errors and merge it before launch?
Or do you think that there is something wrong with the proposed approach and we should think about something different?
Original patch (with per-relation cache) was much simpler but doesn't optimize case with parallel insert/selects from huge table.
I am not sure how popular this workload can be and will per-chunk last written lsn cache be really able to increase performance of such workload.

But first of all it will be nice to understand the reason of this failure...
It may indicate soe other error not directly related with last written LSN cache.

hlinnaka · 2022-07-19T15:26:32Z

I can not reproduce the problem with test_recovery failure locally, did you? Or it just happens in CI?

Certainly, if there are such errors and this patch is blocking other patches, then it should be undone. Do you want me to do it? Just reset main branch to 9c99008 and then apply db67809 on top of it?

That works, but requires a force-push. I'd suggest git revert 7517d1cec45224841eac327cad7e0ddc81c734ff 595ac69260719d8d7b43c09ab7dfd8f232542e50, and git rebase -i origin/main to squash the two revert commits into one.

Yeah, please go ahead and do the revert. Let's unblock the uuid-ossp change, and get vendor/postgres back in sync with the postgres main-branch again.

Also I have implemented version of last written LSN cache which uses buffer tag as a key: https://github.com/neondatabase/postgres/tree/last_written_lsn_buftag_cache I still think that it is enough to have relnode as a key, because it makes comparison/hash calculation operations several times faster. And probability of collision is small. But if you think that it is more important to avoid conflicts we can use this branch.

It's more about readability; it's surprising to use just the relfilenode oid as the key. I don't feel strongly about it, but unless you can see a measurable, significant performance difference, I would prefer using the buffer tag.

So, if we undo this commits, what do you think will be the next steps with this PR? Should we try to fix errors and merge it before launch? Or do you think that there is something wrong with the proposed approach and we should think about something different? Original patch (with per-relation cache) was much simpler but doesn't optimize case with parallel insert/selects from huge table. I am not sure how popular this workload can be and will per-chunk last written lsn cache be really able to increase performance of such workload.

I think the high-level approach here makes sense, we're just talking about the details.

But first of all it will be nice to understand the reason of this failure... It may indicate soe other error not directly related with last written LSN cache.

Agreed. It certainly looks like an LSN-cache related bug, but in any case we need to find the bug.

knizhnik · 2022-07-19T19:33:07Z

I tried to run CI test with this postgres versions.
2 tests are failed in debug build: test_tenant_relocation and test_wal_acceptor.
Both are failed with timeouts and looks like their failure has no relation to this patch.
@hlinnaka are you sure that test_recovery.py on the latest version of this PR and we should undo it?

This reverts commit 7517d1c. Revert "Large last written lsn cache (#177)" This reverts commit 595ac69.

aome510 · 2022-07-19T19:58:34Z

2 tests are failed in debug build: test_tenant_relocation and test_wal_acceptor.
Both are failed with timeouts and looks like their failure has no relation to this patch.

One possible cause is that this patch slows down the performance of those tests. That said, I wasn't able to reproduce the failures locally when running tests individually. Maybe, running in parallel would be different.

The same tests also fail in https://app.circleci.com/pipelines/github/neondatabase/neon/8175/workflows/96416e07-8028-46d0-94d7-2a470ae42425/jobs/83642. They're consistent, so definitely there is something wrong here.