Skip to content

Cleanup of Postgres pre-start code#2352

Merged
rkoster merged 6 commits intomasterfrom
pg-prestart-cleanup
Jan 27, 2022
Merged

Cleanup of Postgres pre-start code#2352
rkoster merged 6 commits intomasterfrom
pg-prestart-cleanup

Conversation

@bgandon
Copy link
Copy Markdown
Contributor

@bgandon bgandon commented Jan 25, 2022

Hello dear Bosh fellows,

Turns out the Postgres pre-start scripts were executed twice: first by Bosh Agent as root on the VM with no restrictions, second inside the BPM context. This issue comes from commit 4ab3193 in the first place, maybe due to lack of review.

When fixing this unnecessary duplicate execution of pre-start scripts, then mounting the old persistent store directory with BPM is no more necessary. This is useful to prevent an empty postgres-10 directory to re-appear at every startup, only because it is declared in BPM for purposes of migrating from v10 to v13.

I've also fixed some bugs related to typos, as the actual directory for Postgres 9.4 is postgres-9.4 and not postgres-9. This was preventing Postgres 10 pre-start script from was properly removing the Postgres 9.4 old persistent data after successful upgrade.

Also, after we've merged #2334, some data repair code in migration from Postgres v9.4 to v10 (due to improper signal being previously used to stop Postgres) is no more necessary, so I've removed it.

Less importantly, I've reviewed the pre-start scripts for the 3 Postgres jobs. I've noticed that the word 'data dir' was used for persistent data, that go to the 'store' dir in Bosh parlance, whereas 'data' dir usually designates the ephemeral data. So, I'm proposing some improved wording in that regards, for cleaner code.

Best,
Benjamin

Copy link
Copy Markdown
Contributor

@rkoster rkoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few missing quotes here an there


mkdir -p $DATA_DIR/pg_log
chown vcap:vcap $DATA_DIR/pg_log
mkdir -p ${STORE_DIR}/pg_log
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing quotes

fi

echo "host all $USER 0.0.0.0/0 md5" >> $DATA_DIR/pg_hba.conf
echo "host all $USER 0.0.0.0/0 md5" >> ${STORE_DIR}/pg_hba.conf
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing quotes

@rkoster rkoster requested a review from lnguyen January 27, 2022 11:34
@bgandon bgandon force-pushed the pg-prestart-cleanup branch from da55c84 to 0621a51 Compare January 27, 2022 11:45
@bgandon
Copy link
Copy Markdown
Contributor Author

bgandon commented Jan 27, 2022

Thank you Ruben for the good catches!
I've extended the scope of the PR quite a lot today, which will require some more reviewing from you unfortunately.

Copy link
Copy Markdown
Contributor

@rkoster rkoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM

Copy link
Copy Markdown
Member

@lnguyen lnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rkoster rkoster merged commit da56d19 into master Jan 27, 2022
@rkoster
Copy link
Copy Markdown
Contributor

rkoster commented Jan 27, 2022

Thanks @bgandon

@rkoster rkoster deleted the pg-prestart-cleanup branch January 27, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants