Undo walloging replorgin file on checkpoint

[knizhnik]

Problem

See #8620

Summary of changes

Remove walloping of replorigin file because it is reconstructed by PS

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

3787 tests run: 3681 passed, 0 failed, 106 skipped (full report)

---

Code coverage* (full report)

• functions: 32.5% (7402 of 22759 functions)
• lines: 50.7% (60052 of 118528 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
e283125 at 2024-08-28T20:50:26.639Z :recycle:}

[hlinnaka]

~/git-sandbox/neon/vendor/postgres-v16 (remove_replorigin_checkpoint_v16)$ git diff REL_16_3 src/backend/replication/logical/origin.c
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index b0255ffd25a..bc540521a96 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -83,6 +83,7 @@
 #include "nodes/execnodes.h"
 #include "pgstat.h"
 #include "replication/logical.h"
+#include "replication/message.h"
 #include "replication/origin.h"
 #include "storage/condition_variable.h"
 #include "storage/copydir.h"
@@ -657,7 +658,6 @@ CheckPointReplicationOrigin(void)
                                         errmsg("could not write to file \"%s\": %m",
                                                        tmppath)));
                }
-
                COMP_CRC32C(crc, &disk_state, sizeof(disk_state));
        }
 
@@ -1144,6 +1144,7 @@ replorigin_session_setup(RepOriginId node, int acquired_by)
 
                /* ok, found slot */
                session_replication_state = curstate;
+               break;
        }
 
 

Why the added break? And let's remove those other superfluous changes too.

[hlinnaka]

Do we have a plan for how to eventually remove those obsolete pg_logical/replorigin_checkpoint files from the storage?

[knizhnik]

~/git-sandbox/neon/vendor/postgres-v16 (remove_replorigin_checkpoint_v16)$ git diff REL_16_3 src/backend/replication/logical/origin.c
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index b0255ffd25a..bc540521a96 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -83,6 +83,7 @@
 #include "nodes/execnodes.h"
 #include "pgstat.h"
 #include "replication/logical.h"
+#include "replication/message.h"
 #include "replication/origin.h"
 #include "storage/condition_variable.h"
 #include "storage/copydir.h"
@@ -657,7 +658,6 @@ CheckPointReplicationOrigin(void)
                                         errmsg("could not write to file \"%s\": %m",
                                                        tmppath)));
                }
-
                COMP_CRC32C(crc, &disk_state, sizeof(disk_state));
        }
 
@@ -1144,6 +1144,7 @@ replorigin_session_setup(RepOriginId node, int acquired_by)
 
                /* ok, found slot */
                session_replication_state = curstate;
+               break;
        }
 
 

Why the added break? And let's remove those other superfluous changes too.

Sorry, removed unintended changes.

[knizhnik]

Do we have a plan for how to eventually remove those obsolete pg_logical/replorigin_checkpoint files from the storage?

Any reason for it? Can extra ~100 bytes somehow compensate efforts needed to remove them?

[hlinnaka]

Do we have a plan for how to eventually remove those obsolete pg_logical/replorigin_checkpoint files from the storage?

Any reason for it? Can extra ~100 bytes somehow compensate efforts needed to remove them?

It's not urgent, but it might cause surprises later, after everyone has already forgotten about them.

[hlinnaka]

Why the added break? And let's remove those other superfluous changes too.

Sorry, removed unintended changes.

I can still see the extra 'break' there.

[knizhnik]

Why the added break? And let's remove those other superfluous changes too.

Sorry, removed unintended changes.

I can still see the extra 'break' there.

Sorry, looks like I didn't push changes in neon repo.
Fixed.

[hlinnaka]

A few more trivial remnants of this:

$ git diff REL_16_4..origin/pr/472  src/backend/replication/logical/origin.c
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index b0255ffd25a..739030a87e1 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -83,6 +83,7 @@
 #include "nodes/execnodes.h"
 #include "pgstat.h"
 #include "replication/logical.h"
+#include "replication/message.h"
 #include "replication/origin.h"
 #include "storage/condition_variable.h"
 #include "storage/copydir.h"
@@ -615,6 +616,7 @@ CheckPointReplicationOrigin(void)
                                 errmsg("could not write to file \"%s\": %m",
                                                tmppath)));
        }
+
        COMP_CRC32C(crc, &magic, sizeof(magic));
 
        /* prevent concurrent creations/drops */

Other than that, LGTM