Thread: Re: [HACKERS] Rewriting the test of pg_upgrade as a TAP test
On Thu, Apr 6, 2017 at 7:48 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost@snowman.net> wrote: >> * Michael Paquier (michael.paquier@gmail.com) wrote: >>> 1) Initialize the old cluster and start it. >>> 2) create a bunch of databases with full range of ascii characters. >>> 3) Run regression tests. >>> 4) Take dump on old cluster. >>> 4) Stop the old cluster. >>> 5) Initialize the new cluster. >>> 6) Run pg_upgrade. >>> 7) Start new cluster. >>> 8) Take dump from it. >>> 9) Run deletion script (Oops forgot this part!) >> >> Presumably the check to match the old dump against the new one is also >> performed? > > Yes. That's run with command_ok() at the end. Attached is an updated patch to use --no-sync with pg_dumpall calls. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Thu, Apr 6, 2017 at 7:48 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > On Wed, Apr 5, 2017 at 10:24 PM, Stephen Frost <sfrost@snowman.net> wrote: > >> * Michael Paquier (michael.paquier@gmail.com) wrote: > >>> 1) Initialize the old cluster and start it. > >>> 2) create a bunch of databases with full range of ascii characters. > >>> 3) Run regression tests. > >>> 4) Take dump on old cluster. > >>> 4) Stop the old cluster. > >>> 5) Initialize the new cluster. > >>> 6) Run pg_upgrade. > >>> 7) Start new cluster. > >>> 8) Take dump from it. > >>> 9) Run deletion script (Oops forgot this part!) > >> > >> Presumably the check to match the old dump against the new one is also > >> performed? > > > > Yes. That's run with command_ok() at the end. > > Attached is an updated patch to use --no-sync with pg_dumpall calls. Some of those were specifically left around to test those code paths. I'm not sure if these were those or not though, Andrew was the one who reviewed the various pg_dumpall calls to add --no-sync in places. Thanks! Stephen
On Fri, Apr 14, 2017 at 8:03 PM, Stephen Frost <sfrost@snowman.net> wrote: > Some of those were specifically left around to test those code paths. > I'm not sure if these were those or not though, Andrew was the one who > reviewed the various pg_dumpall calls to add --no-sync in places. Well, Andrew has pushed the patch I have written, and the calls of pg_dumpall in pg_upgrade use --no-sync. The ones intentionally left are in src/bin/pg_dump/t/002_pg_dump.pl. -- Michael
* Michael Paquier (michael.paquier@gmail.com) wrote: > On Fri, Apr 14, 2017 at 8:03 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Some of those were specifically left around to test those code paths. > > I'm not sure if these were those or not though, Andrew was the one who > > reviewed the various pg_dumpall calls to add --no-sync in places. > > Well, Andrew has pushed the patch I have written, and the calls of > pg_dumpall in pg_upgrade use --no-sync. The ones intentionally left > are in src/bin/pg_dump/t/002_pg_dump.pl. Ok. :) Thanks! Stephen
On 4/14/17 02:00, Michael Paquier wrote: > Attached is an updated patch to use --no-sync with pg_dumpall calls. Please send a rebased patch. I propose splitting the single Perl script into three separate test files: one for basic command-line option handling and so on (I would like to expand that later), one for the main upgrade test, and one for the funny database names tests. In the testing file, you removed the paragraph that explains how to do cross-version upgrade testing. It's unfortunate that we would lose that functionality. What can we do about that? We also need to have a plan for handling the build farm. Maybe keep the vcregress.pl upgradecheck target as a thin wrapper for the time being? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > I propose splitting the single Perl script into three separate test > files: one for basic command-line option handling and so on (I would > like to expand that later), one for the main upgrade test, and one for > the funny database names tests. Check. > We also need to have a plan for handling the build farm. Maybe keep the > vcregress.pl upgradecheck target as a thin wrapper for the time being? The buildfarm already runs "make check" in src/bin/ when TAP tests are enabled, which should be enough to trigger the rewritten test, no? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Peter Eisentraut wrote:
>> We also need to have a plan for handling the build farm. Maybe keep the
>> vcregress.pl upgradecheck target as a thin wrapper for the time being?
> The buildfarm already runs "make check" in src/bin/ when TAP tests are
> enabled, which should be enough to trigger the rewritten test, no?
I think Peter's on about the Windows case. Not sure how that's handled,
but it's not "make check".
regards, tom lane
On Wed, Sep 6, 2017 at 11:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> Peter Eisentraut wrote: >>> We also need to have a plan for handling the build farm. Maybe keep the >>> vcregress.pl upgradecheck target as a thin wrapper for the time being? > >> The buildfarm already runs "make check" in src/bin/ when TAP tests are >> enabled, which should be enough to trigger the rewritten test, no? > > I think Peter's on about the Windows case. Not sure how that's handled, > but it's not "make check". For MSVC, one can use "vcregress.bat upgradecheck". So perhaps we could keep upgradecheck for a short time but make it a noop instead with this patch, and then remove it once buildfarm animals are upgraded to a newer client version? I would prefer seeing a simple removal of upgradecheck at the end, and put all TAP tests for binaries under the bincheck path. This feels more natural. -- Michael
On Wed, Sep 6, 2017 at 10:05 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Please send a rebased patch.
>
> I propose splitting the single Perl script into three separate test
> files: one for basic command-line option handling and so on (I would
> like to expand that later), one for the main upgrade test, and one for
> the funny database names tests.
That makes sense. There will be additional overhead with the creation
of an extra server though.
> In the testing file, you removed the paragraph that explains how to do
> cross-version upgrade testing. It's unfortunate that we would lose that
> functionality. What can we do about that?
Right, simply removing support for something which has been here for a
long time is no fun. I think that we should add in PostgresNode
objects a new bindir variable which will be used to define path to
binaries. Any new node created needs to go through init() or
init_from_backup(), so a node created with init() would set this
bindir to what pg_config in PATH reports, or to the value defined by
the caller if it is defined (let's use an option for %params). A node
created from init_from_backup() inherits the path of its root node.
This requires a bit of refactoring first. This could help also for
cross version tests out of the code core.
In the existing scripts, there are the following variables:
- oldsrc, old version's source tree
- oldbindir, old version's installed bin dir
- bindir, this version's installed bin dir.
- libdir, this version's installed lib dir
bindir and libdir are pointing to the currently installed version in
the tree, so we could do without it, no? oldbindir and oldsrc need to
be kept around to enforce the position of binaries for the old
version, as well as a proper shape of the original dump being compared
(+ drop of the past functions).
Then, for the pg_upgrade tests, let's check for ENV{oldbindir} and
enforce the bindir value of the PostgresNode to-be-upgraded. And also
for ENV{oldsrc}, first check if it is defined, and then do the
existing psql/dump changes. So one, in order to run cross-version
checks, would just need to rely on the fact that the version where
installcheck runs is the new version. Does that sound acceptable?
Looking at 5bab198, those don't run that often, but I definitely agree
that breaking something for no apparent reason is not cool either ;p
> We also need to have a plan for handling the build farm. Maybe keep the
> vcregress.pl upgradecheck target as a thin wrapper for the time being?
Or we could make upgradecheck a noop, then remove it once all the MSVC
animals have upgraded to a newer version of the buildfarm client which
does not use upgradecheck anymore (I am fine to send a patch or a pull
request to Andrew for that).
--
Michael
On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Or we could make upgradecheck a noop, then remove it once all the MSVC > animals have upgraded to a newer version of the buildfarm client which > does not use upgradecheck anymore (I am fine to send a patch or a pull > request to Andrew for that). This patch is logged as "waiting on author" in the current commit fest, but any new patch will depend on the feedback that any other hacker has to offer based on the set of ideas I have posted upthread. Hence I am yet unsure what is the correct way to move things forward. So, any opinions? Peter or others? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier wrote: > On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > Or we could make upgradecheck a noop, then remove it once all the MSVC > > animals have upgraded to a newer version of the buildfarm client which > > does not use upgradecheck anymore (I am fine to send a patch or a pull > > request to Andrew for that). > > This patch is logged as "waiting on author" in the current commit > fest, but any new patch will depend on the feedback that any other > hacker has to offer based on the set of ideas I have posted upthread. > Hence I am yet unsure what is the correct way to move things forward. > So, any opinions? Peter or others? I think the first step is to send the rebased version of the patch. It was last posted in April ... -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 6:15 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Michael Paquier wrote: >> On Thu, Sep 7, 2017 at 11:14 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >> > Or we could make upgradecheck a noop, then remove it once all the MSVC >> > animals have upgraded to a newer version of the buildfarm client which >> > does not use upgradecheck anymore (I am fine to send a patch or a pull >> > request to Andrew for that). >> >> This patch is logged as "waiting on author" in the current commit >> fest, but any new patch will depend on the feedback that any other >> hacker has to offer based on the set of ideas I have posted upthread. >> Hence I am yet unsure what is the correct way to move things forward. >> So, any opinions? Peter or others? > > I think the first step is to send the rebased version of the patch. It > was last posted in April ... Here you go. I have not done anything fancy for cross-version tests yet. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 9/19/17 07:37, Michael Paquier wrote: >>> This patch is logged as "waiting on author" in the current commit >>> fest, but any new patch will depend on the feedback that any other >>> hacker has to offer based on the set of ideas I have posted upthread. >>> Hence I am yet unsure what is the correct way to move things forward. >>> So, any opinions? Peter or others? >> >> I think the first step is to send the rebased version of the patch. It >> was last posted in April ... > > Here you go. I have not done anything fancy for cross-version tests yet. To get things rolling, I have committed just the basic TAP tests, to give it a spin on the build farm. I'll work through the rest in the coming days. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > To get things rolling, I have committed just the basic TAP tests, to > give it a spin on the build farm. I'll work through the rest in the > coming days. Thanks! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/19/17 19:00, Michael Paquier wrote: > On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> To get things rolling, I have committed just the basic TAP tests, to >> give it a spin on the build farm. I'll work through the rest in the >> coming days. I have reverted this because of the build farm issue. Putting the patch on hold in the CF until we have a new plan. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/22/17 16:48, Peter Eisentraut wrote: > On 9/19/17 19:00, Michael Paquier wrote: >> On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> To get things rolling, I have committed just the basic TAP tests, to >>> give it a spin on the build farm. I'll work through the rest in the >>> coming days. > > I have reverted this because of the build farm issue. Putting the patch > on hold in the CF until we have a new plan. Set to "Returned with feedback" now. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers