11 ms·

Important PostgreSQL 14 update to avoid silent corruption of indexes

330 points by avi_vallarapu 4y ago

Linda703 4y ago
[dead]
avi_vallarapu 4y ago
pg_amcheck can be helpful for validation if its a b-tree index
- ioltas 4y ago
  FWIW, amcheck can be useful but I am not 100% sure that it can detect all the broken cases either.
cesarb 4y ago
A more official announcement seems to be at https://www.postgresql.org/message-id/165473835807.573551.1512237163040609764%40wrigleys.postgresql.org https://www.postgresql.org/message-id/165473835807.573551.15...
taspeotis 4y ago
I think this is the fix: https://github.com/postgres/postgres/commit/e28bb885196916b0a3d898ae4f2be0e38108d81b https://github.com/postgres/postgres/commit/e28bb885196916b0... Discussion: https://www.postgresql.org/message-id/17485-396609c6925b982d%40postgresql.org https://www.postgresql.org/message-id/17485-396609c6925b982d...
mattashii 4y ago
It is noted in the post, I'll repeat it to be clear: This corruption can only occur _during_ a (re)index with CONCURRENTLY specified, on rows that are modified during the reindex operation, and only for that index. No other indexes are impacted, and an index can only be impacted when the updates on the table don't update indexed columns. Nevertheless, if you frequently run CIC, you could be having this issue -- right now you can detect the issue with amcheck, and fix it with a non-concurrent REINDEX of the index (yes, this locks the table). Alternatively (not listed in the blog post, but possible if you can't afford table locks), you should be able to safely CIC (without corruption) by doing manual snapshot control while concurrently reindexing the index (takes 3 database sessions): In session 1 start a REPEATABLE READ read-only transaction. In session 2, start the concurrent (re)index. In session 3, monitor pg_stat_progress_create_index for session 2 to get to a 'waiting for ...' phase. When you detect the (re)index session arrives in a waiting phase, start a REPEATABLE READ transaction in this session, and then halt (commit or rollback doesn't matter) the transaction in session 1. Now, we switch roles of sessions 1 and 3, and repeat this lock-step while the session that is (re)indexing hasn't completed.
- mattashii 4y ago
  I'll note that this process is basically keeping a snapshot that ensures that the data CIC/RIC needs is not cleaned up during the critical phases of the reindex process by keeping the cleanup horizon at a point where CIC/RIC can see all the versions of the rows that it needs to create the full index. For PG14 the maintaining of that horizon was disabled for backends that run CIC/RIC, resulting in this bug. If you manually keep that horizon from moving using other backends on the same database, the old versions that CIC/RIC expects will not be removed, and thus no corruption will occur.
- throwawaydingus 4y ago
  What? That seems absurdly apologist even for me and I love postgres. Everyone seems to be downplaying this corruption issue saying it only happens when x or y and it can be avoided by doing complex operation z but… if build anything that isn’t a toy or demo on Postgres you NEED to build indexes concurrently. They shipped optimization to a feature to let indexes be built concurrently yet failed to perform thorough enough testing to ensure concurrent updates don’t disrupt the correctness of the index? Yikes, doesn't exactly make me confident in using any new Postgres features. Further, there’s no workaround that wouldn’t cause a production outage due to requiring a table lock until they release the new build which is somehow not as soon as it’s available. How are we not more alarmed by the impact of this?
  - mattashii 4y ago
    > Everyone seems to be downplaying this corruption issue saying it only happens when x or y and it can be avoided by doing complex operation z but… I don't say corruption is not happening, but I clarify that you might not be affected: this corruption is not as "silent" as the title of the original post would make you think (it only occurs during concurrent reindex/index creation, which is not something that occurs naturally). > if build anything that isn’t a toy or demo on Postgres you NEED to build indexes concurrently I fully disagree. Many databases of customers I've seen are <5GB in size (largest table ~800MB) and used by latency-sensitive workloads only 8 hours each day, while also being mostly read-only. Locking DDL outside that 8-hour window would be perfectly fine, and even within that window non-concurrent reindex generally would have been OK - sorting 800MB does take some 10 seconds, but that is only a small hiccup for data modifying workloads. > Further, there’s no workaround that wouldn’t cause a production outage due to requiring a table lock until they release the new build which is somehow not as soon as it’s available. As my comment describes, that is incorrect. If you carefully maintain snapshots in concurrent connections on that database, you can safely reindex the table concurrently while having as many locks on the table as PG13 had. These connections do not need to hold a lock on the target table, they only need to register and hold their snapshot at the right times.
msk20 4y ago
Looking at the file with changes https://github.com/postgres/postgres/blob/master/src/backend/storage/ipc/procarray.c https://github.com/postgres/postgres/blob/master/src/backend... , I have to say this source code repository is so well documented/commented and structured, I really gives you a huge trust in postgres to be used in your stack.
- colordrops 4y ago
  File could definitely be broken up a bit. Over 5000 lines! Just a nitpick though.
  - RcouF1uZ4gsC 4y ago
    Actually I don’t mind big files. It is simpler scanning through it or doing a quick search than if you had a bunch of smaller files. And 5000 lines is not awkward for most editors, especially as many editors have the ability to collapse functions.
    - eyelidlessness 4y ago
      This feels like a good area for tooling (editors, source hosts, SCM extensions) to improve experience. I don’t always mind large source files (and sometimes may prefer them over large file system hierarchies), but the can be a pain to navigate in some circumstances. As an example, making several related changes in very different parts of a file, where you need to cross-reference between them. The changes themselves might be small, but it’s a huge cognitive burden to alternate/iterate through them. I’d love to have a view which temporarily projects those targets as if they’re isolated files without changing the actual structure on disk. I’d love it so much I actually do this manually for a lot of tasks, creating temp files to prepare edits for related areas of code. But then I lose a lot of the benefits of tools which understand what’s being referenced. It would be great to just type a quick command (or click or whatever) to say “don’t refactor this function to another file, but let’s pretend you did, for a while”.
      - 4y ago
deleted 4y ago
[deleted]
boiler_up800 4y ago
It’s impressive that this is a single revert. That speaks to how the development of Postgres is done atomically. Also not surprised to see it was the EDB team with the expertise to fix it. Their model is a little outdated but they have a lot of experts working there.
- gurjeet 4y ago
  I'm not sure who you're referring to as the EDB team. Please share what make you think that. The commit [1] (dug up by someone else in this discussion), references 4 people (including the committer), and only one of them seems to be EDB employee. (Info gleaned from their respective LinkedIn profiles, and from email signatures sent to pgsql-hackers list) [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e28bb885196916b0a3d898ae4f2be0e38108d81b https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit...
  - mattashii 4y ago
    All of those mentioned in the commit helped in the diagnosis, while Andres was the one to detail the full chain of events that describes how the corruption happened, in [0]. [0] https://www.postgresql.org/message-id/20220524190133.j6ee7zh4f5edt5je%40alap3.anarazel.de https://www.postgresql.org/message-id/20220524190133.j6ee7zh...
deleted 4y ago
[deleted]
TedShiller 4y ago
I love Postgres. But when I look at the bug list of every release it makes me scared. The types of bugs they have are indicative of a poor development process.
- throwawaylinux 4y ago
  What types of bugs are those? Can you explain a bit more?
- jtc331 4y ago
  No, it's indicative of the complexity of the system. Please show other similarly complex systems with fewer bugs: basically doesn't happen.
  - Underphil 4y ago
    Yep. We all have dirty laundry, it's just that we can see it open-source projects.
    - wiz21c 4y ago
      And at least, we see it. It's much easier to appreciate the threat when you see the submerged part of the iceberg :-)
- avi_vallarapu 4y ago
  Can you name a few such bugs please?
- avi_vallarapu 4y ago
  I worked on Oracle and MySQL for ages and seen tons of bugs. As a support company, the number of bugs we encounter with Postgres are none or hardly 1 in an year. I am still curios to see what those bugs are that made one scary !!!
  - jabiko 4y ago
    Yeah, ORA-00600 comes to mind. At my previous company we had to restore from backups once because of this error.
optimalsolver 4y ago
I'm running 12.11. Do I need to do anything?
- hipokampa 4y ago
  an upgrade
  - optimalsolver 4y ago
    The folks on 14 probably wish they held off for a bit. :-D
- x4m 4y ago
  You are running on up to date version which seems to have no known critical bugs.
cfernhout 4y ago
Awesome update!

330 points by avi_vallarapu 4y ago

Linda703 4y ago

[dead]

avi_vallarapu 4y ago

pg_amcheck can be helpful for validation if its a b-tree index

ioltas 4y ago
FWIW, amcheck can be useful but I am not 100% sure that it can detect all the broken cases either.

cesarb 4y ago

A more official announcement seems to be at https://www.postgresql.org/message-id/165473835807.573551.1512237163040609764%40wrigleys.postgresql.org https://www.postgresql.org/message-id/165473835807.573551.15...

taspeotis 4y ago

I think this is the fix: https://github.com/postgres/postgres/commit/e28bb885196916b0a3d898ae4f2be0e38108d81b https://github.com/postgres/postgres/commit/e28bb885196916b0... Discussion: https://www.postgresql.org/message-id/17485-396609c6925b982d%40postgresql.org https://www.postgresql.org/message-id/17485-396609c6925b982d...

mattashii 4y ago

It is noted in the post, I'll repeat it to be clear: This corruption can only occur _during_ a (re)index with CONCURRENTLY specified, on rows that are modified during the reindex operation, and only for that index. No other indexes are impacted, and an index can only be impacted when the updates on the table don't update indexed columns. Nevertheless, if you frequently run CIC, you could be having this issue -- right now you can detect the issue with amcheck, and fix it with a non-concurrent REINDEX of the index (yes, this locks the table). Alternatively (not listed in the blog post, but possible if you can't afford table locks), you should be able to safely CIC (without corruption) by doing manual snapshot control while concurrently reindexing the index (takes 3 database sessions): In session 1 start a REPEATABLE READ read-only transaction. In session 2, start the concurrent (re)index. In session 3, monitor pg_stat_progress_create_index for session 2 to get to a 'waiting for ...' phase. When you detect the (re)index session arrives in a waiting phase, start a REPEATABLE READ transaction in this session, and then halt (commit or rollback doesn't matter) the transaction in session 1. Now, we switch roles of sessions 1 and 3, and repeat this lock-step while the session that is (re)indexing hasn't completed.

mattashii 4y ago
I'll note that this process is basically keeping a snapshot that ensures that the data CIC/RIC needs is not cleaned up during the critical phases of the reindex process by keeping the cleanup horizon at a point where CIC/RIC can see all the versions of the rows that it needs to create the full index. For PG14 the maintaining of that horizon was disabled for backends that run CIC/RIC, resulting in this bug. If you manually keep that horizon from moving using other backends on the same database, the old versions that CIC/RIC expects will not be removed, and thus no corruption will occur.
throwawaydingus 4y ago
What? That seems absurdly apologist even for me and I love postgres. Everyone seems to be downplaying this corruption issue saying it only happens when x or y and it can be avoided by doing complex operation z but… if build anything that isn’t a toy or demo on Postgres you NEED to build indexes concurrently. They shipped optimization to a feature to let indexes be built concurrently yet failed to perform thorough enough testing to ensure concurrent updates don’t disrupt the correctness of the index? Yikes, doesn't exactly make me confident in using any new Postgres features. Further, there’s no workaround that wouldn’t cause a production outage due to requiring a table lock until they release the new build which is somehow not as soon as it’s available. How are we not more alarmed by the impact of this?
- mattashii 4y ago
  > Everyone seems to be downplaying this corruption issue saying it only happens when x or y and it can be avoided by doing complex operation z but… I don't say corruption is not happening, but I clarify that you might not be affected: this corruption is not as "silent" as the title of the original post would make you think (it only occurs during concurrent reindex/index creation, which is not something that occurs naturally). > if build anything that isn’t a toy or demo on Postgres you NEED to build indexes concurrently I fully disagree. Many databases of customers I've seen are <5GB in size (largest table ~800MB) and used by latency-sensitive workloads only 8 hours each day, while also being mostly read-only. Locking DDL outside that 8-hour window would be perfectly fine, and even within that window non-concurrent reindex generally would have been OK - sorting 800MB does take some 10 seconds, but that is only a small hiccup for data modifying workloads. > Further, there’s no workaround that wouldn’t cause a production outage due to requiring a table lock until they release the new build which is somehow not as soon as it’s available. As my comment describes, that is incorrect. If you carefully maintain snapshots in concurrent connections on that database, you can safely reindex the table concurrently while having as many locks on the table as PG13 had. These connections do not need to hold a lock on the target table, they only need to register and hold their snapshot at the right times.

msk20 4y ago

Looking at the file with changes https://github.com/postgres/postgres/blob/master/src/backend/storage/ipc/procarray.c https://github.com/postgres/postgres/blob/master/src/backend... , I have to say this source code repository is so well documented/commented and structured, I really gives you a huge trust in postgres to be used in your stack.

colordrops 4y ago
File could definitely be broken up a bit. Over 5000 lines! Just a nitpick though.
- RcouF1uZ4gsC 4y ago
  Actually I don’t mind big files. It is simpler scanning through it or doing a quick search than if you had a bunch of smaller files. And 5000 lines is not awkward for most editors, especially as many editors have the ability to collapse functions.
  - eyelidlessness 4y ago
    This feels like a good area for tooling (editors, source hosts, SCM extensions) to improve experience. I don’t always mind large source files (and sometimes may prefer them over large file system hierarchies), but the can be a pain to navigate in some circumstances. As an example, making several related changes in very different parts of a file, where you need to cross-reference between them. The changes themselves might be small, but it’s a huge cognitive burden to alternate/iterate through them. I’d love to have a view which temporarily projects those targets as if they’re isolated files without changing the actual structure on disk. I’d love it so much I actually do this manually for a lot of tasks, creating temp files to prepare edits for related areas of code. But then I lose a lot of the benefits of tools which understand what’s being referenced. It would be great to just type a quick command (or click or whatever) to say “don’t refactor this function to another file, but let’s pretend you did, for a while”.
    - 4y ago

deleted 4y ago

[deleted]

boiler_up800 4y ago

It’s impressive that this is a single revert. That speaks to how the development of Postgres is done atomically. Also not surprised to see it was the EDB team with the expertise to fix it. Their model is a little outdated but they have a lot of experts working there.

gurjeet 4y ago
I'm not sure who you're referring to as the EDB team. Please share what make you think that. The commit [1] (dug up by someone else in this discussion), references 4 people (including the committer), and only one of them seems to be EDB employee. (Info gleaned from their respective LinkedIn profiles, and from email signatures sent to pgsql-hackers list) [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e28bb885196916b0a3d898ae4f2be0e38108d81b https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit...
- mattashii 4y ago
  All of those mentioned in the commit helped in the diagnosis, while Andres was the one to detail the full chain of events that describes how the corruption happened, in [0]. [0] https://www.postgresql.org/message-id/20220524190133.j6ee7zh4f5edt5je%40alap3.anarazel.de https://www.postgresql.org/message-id/20220524190133.j6ee7zh...

deleted 4y ago

[deleted]

TedShiller 4y ago

I love Postgres. But when I look at the bug list of every release it makes me scared. The types of bugs they have are indicative of a poor development process.

throwawaylinux 4y ago
What types of bugs are those? Can you explain a bit more?
jtc331 4y ago
No, it's indicative of the complexity of the system. Please show other similarly complex systems with fewer bugs: basically doesn't happen.
- Underphil 4y ago
  Yep. We all have dirty laundry, it's just that we can see it open-source projects.
  - wiz21c 4y ago
    And at least, we see it. It's much easier to appreciate the threat when you see the submerged part of the iceberg :-)
avi_vallarapu 4y ago
Can you name a few such bugs please?
avi_vallarapu 4y ago
I worked on Oracle and MySQL for ages and seen tons of bugs. As a support company, the number of bugs we encounter with Postgres are none or hardly 1 in an year. I am still curios to see what those bugs are that made one scary !!!
- jabiko 4y ago
  Yeah, ORA-00600 comes to mind. At my previous company we had to restore from backups once because of this error.

optimalsolver 4y ago

I'm running 12.11. Do I need to do anything?

hipokampa 4y ago
an upgrade
- optimalsolver 4y ago
  The folks on 14 probably wish they held off for a bit. :-D
x4m 4y ago
You are running on up to date version which seems to have no known critical bugs.

cfernhout 4y ago

Awesome update!