Обсуждение: pg_upgrade --clone error checking
With the new pg_upgrade --clone, if we are going to end up throwing the error "file cloning not supported on this platform" (which seems to depend only on ifdefs) I think we should throw it first thing, before any other checks are done and certainly before pg_dump gets run.
This might result in some small amount of code duplication, but I think it would be worth the cost.
For cases where we might throw "could not clone file between old and new data directories", I wonder if we shouldn't do some kind of dummy copy to catch that error earlier, as well. Maybe that one is not worth it.
Cheers,
Jeff
On 2019-05-01 22:10, Jeff Janes wrote: > With the new pg_upgrade --clone, if we are going to end up throwing the > error "file cloning not supported on this platform" (which seems to > depend only on ifdefs) I think we should throw it first thing, before > any other checks are done and certainly before pg_dump gets run. Could you explain in more detail what command you are running, what messages you are getting, and what you would like to see instead? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, May 2, 2019 at 11:57 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 2019-05-01 22:10, Jeff Janes wrote:
> With the new pg_upgrade --clone, if we are going to end up throwing the
> error "file cloning not supported on this platform" (which seems to
> depend only on ifdefs) I think we should throw it first thing, before
> any other checks are done and certainly before pg_dump gets run.
Could you explain in more detail what command you are running, what
messages you are getting, and what you would like to see instead?
I'm running:
pg_upgrade --clone -b /home/jjanes/pgsql/REL9_6_12/bin/ -B /home/jjanes/pgsql/origin_jit/bin/ -d /home/jjanes/pgsql/data_96/ -D /home/jjanes/pgsql/data_clone/
And I get:
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database user is the install user ok
Checking database connection settings ok
Checking for prepared transactions ok
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch ok
Checking for tables WITH OIDS ok
Checking for invalid "unknown" user columns ok
Creating dump of global objects ok
Creating dump of database schemas
ok
Checking for presence of required libraries ok
file cloning not supported on this platform
Failure, exiting
I think the error message wording is OK, I think it should be thrown earlier, before the "Creating dump of database schemas" (which can take a long time), and preferably before either database is even started. So ideally it would be something like:
Performing Consistency Checks
-----------------------------
Checking cluster versions
Checking file cloning support
File cloning not supported on this platform
Failure, exiting
When something is doomed to fail, we should report the failure as early as feasibly detectable.
Cheers,
Jeff
On 2019-May-02, Jeff Janes wrote: > I think the error message wording is OK, I think it should be thrown > earlier, before the "Creating dump of database schemas" (which can take a > long time), and preferably before either database is even started. So > ideally it would be something like: > > > Performing Consistency Checks > ----------------------------- > Checking cluster versions > Checking file cloning support > File cloning not supported on this platform > Failure, exiting > > > When something is doomed to fail, we should report the failure as early as > feasibly detectable. I agree -- this check should be done before checking the database contents. Maybe even before "Checking cluster versions". -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, May 2, 2019 at 12:28 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-May-02, Jeff Janes wrote:
>
> When something is doomed to fail, we should report the failure as early as
> feasibly detectable.
I agree -- this check should be done before checking the database
contents. Maybe even before "Checking cluster versions".
It looks like it was designed for early checking, it just wasn't placed early enough. So changing it is pretty easy, as check_file_clone does not need to be invented, and there is no additional code duplication over what was already there.
This patch moves the checking to near the beginning.
It carries the --link mode checking along with it. That should be done as well, and doing it as a separate patch would make both patches uglier.
Cheers,
Jeff
Вложения
On 2019-05-02 20:03, Jeff Janes wrote: > It looks like it was designed for early checking, it just wasn't placed > early enough. So changing it is pretty easy, as check_file_clone does > not need to be invented, and there is no additional code duplication > over what was already there. > > This patch moves the checking to near the beginning. I think the reason it was ordered that way is that it wants to do all the checks of the old cluster before doing any checks touching the new cluster. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 3, 2019 at 3:53 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 2019-05-02 20:03, Jeff Janes wrote:
> It looks like it was designed for early checking, it just wasn't placed
> early enough. So changing it is pretty easy, as check_file_clone does
> not need to be invented, and there is no additional code duplication
> over what was already there.
>
> This patch moves the checking to near the beginning.
I think the reason it was ordered that way is that it wants to do all
the checks of the old cluster before doing any checks touching the new
cluster.
But is there a reason to want to do that? I understand we don't want to keep starting and stopping the clusters needlessly, so we should do everything we can in one before moving to the other. But for checks that don't need a running cluster, why would it matter? The existence and contents of PG_VERSION of the new cluster directory is already checked at the very beginning (and even tries to start it up and shuts it down again if a pid file also exists), so there is precedence for touching the new cluster directory at the filesystem level early (albeit in a readonly manner) and if a pid file exists then doing even more than that. I didn't move check_file_clone to before the liveness check is done, out of a abundance of caution. But creating a transient file with a name of no significance ("PG_VERSION.clonetest") in a cluster that is not even running seems like a very low risk thing to do. The pay off is that we get an inevitable error message much sooner.
Cheers,
Jeff