[PG-961] automated bash script for replication, expected files and up… #71
Conversation
There was a problem hiding this comment.
This is a good base script, but I see three issues here:
- No verification that data on disk is actually encrypted - not for the primary, not for the replica.
- The data load is very minimal: we create a basic dataset with minimal tables, and then add a few extra SQL commands while the server is running. (1) The dataset should be bigger, currently everything fits into a few kilobytes, that's not realistic. (2) the replication process should run longer, with more changes, maybe even adding pgbench/sysbench there for a dynamic load. (3) The dataset should use more features. We have no indexes, toast tables, views/materalized views and so on.
- The incremental/data load part doesn't change encryption parameters: rotating keys, changing the key provider, moving from default provider to database local, anything really.
Most of these comments are also true for the backup test.
|
Please do not add main -> feature branch merges ("Merge branch 'TDE_REL_17_STABLE' into PG-961") to the PR. When you need changes from the main branch, use the rebase functionality. |
| SCRIPT_DIR="$(cd -- "$(dirname "$0")" >/dev/null 2>&1; pwd -P)" | ||
| INSTALL_DIR="$SCRIPT_DIR/../../pginst" | ||
| INSTALL_DIR="$SCRIPT_DIR/../../pginst/17" |
There was a problem hiding this comment.
This seems to be inconsistent with the rest of the scripts?
|
Yeah this is typo. I was using this path for testing purpose
…On Tue, 25 Feb 2025 at 1:22 PM, Zsolt Parragi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ci_scripts/backup/replication_script.sh
<#71 (comment)>:
> SCRIPT_DIR="$(cd -- "$(dirname "$0")" >/dev/null 2>&1; pwd -P)"
-INSTALL_DIR="$SCRIPT_DIR/../../pginst"
+INSTALL_DIR="$SCRIPT_DIR/../../pginst/17"
This seems to be inconsistent with the rest of the scripts?
—
Reply to this email directly, view it on GitHub
<#71 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHUT657HRMAPT32C3WDNKR32RQR2NAVCNFSM6AAAAABXMYJLXOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMNBQGAZDCMJSGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| -- ELSE | ||
| -- RAISE NOTICE 'Skipping WAL check on Master.'; | ||
| -- END IF; | ||
| --END $$; |
There was a problem hiding this comment.
As the output is different for master and slave node, so it was a bit tricky to handle the output file comparison. Now the script have separate function, which verify lsn on master/slave1/slave2 nodes on the run. The new changes in the script will have this function.
ci_scripts/configure-tde-server.sh
Outdated
| @@ -16,16 +16,16 @@ export PGPORT="${2:-5432}" | |||
|
|
|||
| if [ -d "$PGDATA" ]; then | |||
| if pg_ctl -D "$PGDATA" status -o "-p $PGPORT" >/dev/null; then | |||
| pg_ctl -D "$PGDATA" stop -o "-p $PGPORT" | |||
| pg_ctl -D "$PGDATA" stop -o "-p $PGPORT" >/dev/null; | |||
There was a problem hiding this comment.
I want the output so I can debug the script.
There was a problem hiding this comment.
Yeah agree, it will be difficult to debug. will remove this change.
|
|
||
| psql postgres -f "$SCRIPT_DIR/tde_setup_global.sql" | ||
|
|
||
| pg_ctl -D "$PGDATA" restart -o "-p $PGPORT" | ||
| pg_ctl -D "$PGDATA" restart -o "-p $PGPORT" -l "$PGDATA/logfile" |
There was a problem hiding this comment.
This is a bit tricky. I would not want this behaviour locally but I can see the point of doing it for CI. Too much wirtten to stdout otherwise.
There was a problem hiding this comment.
what do you suggest in this case? Is it okay or should I revert this change?
There was a problem hiding this comment.
@dutow What do you think about this? I am unsure myself.
…and replica nodes.
…ure the script for better results
shahidullah79
force-pushed
the
PG-961
branch
from
d800082 to
9bbee35
Compare
…dated sql
PG-961
Description
A bash script that verify streaming replication. This script following tasks
Links