Remove pg_depend entries from columnar metadata indexes to columnar-am (inserted in #5456) #6628
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
onurctirtir
commented
Jan 18, 2023
| 2 | 0 | 1 | table_with_orphaned_shards_102011 | 0 | 0 | ||
| (2 rows) | ||
|
|
||
| -- TODO: Looks like citus_columnar doesn't get auto-updated when updating Citus |
Member
Author
There was a problem hiding this comment.
ok, but at least open an issue for this
onurctirtir
commented
Jan 18, 2023
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
149318d to
bac5409
Compare
Codecov Report
@@ Coverage Diff @@
## main #6628 +/- ##
==========================================
+ Coverage 93.15% 93.16% +0.01%
==========================================
Files 259 259
Lines 55962 55960 -2
==========================================
+ Hits 52130 52137 +7
+ Misses 3832 3823 -9 |
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
bac5409 to
79bd16b
Compare
onurctirtir
changed the title
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
47a2130 to
4611fed
Compare
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
4611fed to
31c78e3
Compare
onurctirtir
commented
Mar 14, 2023
src/backend/columnar/sql/udfs/columnar_ensure_am_depends_catalog/11.2-1.sql
Outdated
Show resolved
Hide resolved
marcocitus
reviewed
Mar 14, 2023
| @@ -0,0 +1,43 @@ | |||
| CREATE OR REPLACE FUNCTION columnar_internal.columnar_ensure_am_depends_catalog() | |||
Member
Author
There was a problem hiding this comment.
via citus_finish_pg_upgrade()
marcocitus
reviewed
Mar 14, 2023
Contributor
marcocitus
left a comment
marcocitus
left a comment
There was a problem hiding this comment.
Is it possible to have some kind of pg_dump test to confirm we fixed the original issue?
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
642f61c to
c282ec8
Compare
Member
Author
I see that we have some pg_dump tests for columnar in pg_dump.sql but apparently they don't report warnings etc., so not sure if there is way to do that. |
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
5 times, most recently
from
26b101e to
e7aff54
Compare
When run_test.py is run for an upgrade_.*_after.sql then, then automatically run the corresponding uprade_.*_before.sql file first. This is because all those upgrade_.*_after.sql files depend on the objects created in upgrade_.*_before.sql files by definition.
This commit hides port numbers in upgrade_columnar_after because the port numbers assigned to nodes in upgrade schedule differ from the ones that flaky test detector assigns.
…le times So that flaky test detector can run upgrade_columnar_before.sql multiple times.
In the past, having columnar tables in the cluster was causing pg upgrades to fail when attempting to access columnar metadata. This is because, pg_dump doesn't see objects that we use for columnar-am related booking as the dependencies of the tables using columnar-am. To fix that; in #5456, we inserted some "normal dependency" edges (from those objects to columnar-am) into pg_depend. This helped us ensuring the existency of a class of metadata objects --such as columnar.storageid_seq-- and helped fixing #5437. However, the normal-dependency edges that we added for indexes on columnar metadata tables --such columnar.stripe_pkey-- didn't help at all because they were indeed causing dependency loops (#5510) and pg_dump was not able to take those dependency edges into the account. For this reason, this commit deletes those dependency edges so that pg_dump stops complaining about them. Note that it's not critical to delete those edges from pg_depend since they're not breaking pg upgrades but were triggering some warning messages. And given that backporting a sql change into older versions is hard a lot, we skip backporting this.
onurctirtir
force-pushed
the
fix/columnar-dependency-loop
branch
from
e7aff54 to
9550ebd
Compare
marcocitus
approved these changes
Mar 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DESCRIPTION: Fixes (pg_dump/pg_upgrade) dependency loop warnings caused by pg_depend entries inserted by citus_columnar
Fixes #5510.
In the past, having columnar tables in the cluster was causing pg
upgrades to fail when attempting to access columnar metadata. This is
because, pg_dump doesn't see objects that we use for columnar-am related
booking as the dependencies of the tables using columnar-am.
To fix that; in #5456, we inserted some "normal dependency" edges (from
those objects to columnar-am) into pg_depend.
This helped us ensuring the existency of a class of metadata objects
--such as columnar.storageid_seq-- and helped fixing #5437.
However, the normal-dependency edges that we added for indexes on
columnar metadata tables --such columnar.stripe_pkey-- didn't help at
all because they were indeed causing dependency loops (#5510) and
pg_dump was not able to take those dependency edges into the account.
For this reason, this commit deletes those dependency edges so that
pg_dump stops complaining about them. Note that it's not critical to
delete those edges from pg_depend since they're not breaking pg upgrades
but were triggering some warning messages. And given that backporting
a sql change into older versions is hard a lot, we skip backporting
this.