Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Magnus Hagander]

On Thu, Apr 29, 2021 at 4:41 AM Tom Lane  wrote:
>
> Michael Paquier  writes:
> > On Wed, Apr 28, 2021 at 12:44:45PM +0300, Aleksander Alekseev wrote:
> >> I just wanted to let you know that TimescaleDB uses
> >> pg_isolation_regress and occasionally there are reports from some
> >> suffering/puzzled users/developers, e.g. [1]. Not 100% sure if it
> >> makes investing the time into backpatching worth it. However if
> >> someone could do it, it would be nice.
>
> > FWIW, I am not really sure that this is important enough to justify a
> > back-patch, even it is true that there have been cases in the past
> > where extra binaries got added in minor releases.
>
> Yeah, I think adding a binary in a minor release is a Big Deal to
> packagers.  I doubt that the case here justifies that.

+1.

Given the number of complaints from people lacking it since the binary
was first created, I can't see how that's a priority that justifies
that.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Tom Lane]

Michael Paquier  writes:
> On Wed, Apr 28, 2021 at 12:44:45PM +0300, Aleksander Alekseev wrote:
>> I just wanted to let you know that TimescaleDB uses
>> pg_isolation_regress and occasionally there are reports from some
>> suffering/puzzled users/developers, e.g. [1]. Not 100% sure if it
>> makes investing the time into backpatching worth it. However if
>> someone could do it, it would be nice.

> FWIW, I am not really sure that this is important enough to justify a
> back-patch, even it is true that there have been cases in the past
> where extra binaries got added in minor releases.

Yeah, I think adding a binary in a minor release is a Big Deal to
packagers.  I doubt that the case here justifies that.

regards, tom lane

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Michael Paquier]

On Wed, Apr 28, 2021 at 12:44:45PM +0300, Aleksander Alekseev wrote:
> I just wanted to let you know that TimescaleDB uses
> pg_isolation_regress and occasionally there are reports from some
> suffering/puzzled users/developers, e.g. [1]. Not 100% sure if it
> makes investing the time into backpatching worth it. However if
> someone could do it, it would be nice.

FWIW, I am not really sure that this is important enough to justify a
back-patch, even it is true that there have been cases in the past
where extra binaries got added in minor releases.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Aleksander Alekseev]

Hi hackers,

> Any opinions on backpatching?

> Other than src/test/modules/brin, the ISOLATION users don't look
> much like real extensions (rather than test scaffolding), either.

> Out of core, the only thing I can see with isolation tests is rum, but
> it uses a workaround to have an access to the necessary binaries.

I just wanted to let you know that TimescaleDB uses
pg_isolation_regress and occasionally there are reports from some
suffering/puzzled users/developers, e.g. [1]. Not 100% sure if it
makes investing the time into backpatching worth it. However if
someone could do it, it would be nice.

[1]: https://github.com/timescale/timescaledb/issues/1655

-- 
Best regards,
Aleksander Alekseev

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Craig Ringer]

On Fri, 16 Oct 2020, 09:00 Michael Paquier,  wrote:

> On Thu, Oct 15, 2020 at 01:06:54PM -0400, Tom Lane wrote:
> > Other than src/test/modules/brin, the ISOLATION users don't look
> > much like real extensions (rather than test scaffolding), either.
> > If you discount test scaffolding modules then the use-counts are
> > more like 4 to 1.
>
> Out of core, the only thing I can see with isolation tests is rum, but
> it uses a workaround to have an access to the necessary binaries.
>

I would've liked to backpatch but don't really care very much. If it's
going to take time away from others things, don't do it.

I landed up having to make my own lightly customised postgres packages to
use as test workflow inputs anyway. So I included the full set of isolation
test utilities, packaged the test inputs etc.

I'd prefer not to have to do it, but it's done. So long as it's fixed going
forward it didn't matter that much.

Now server_version_num on the other hand ... :P

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Michael Paquier]

On Thu, Oct 15, 2020 at 01:06:54PM -0400, Tom Lane wrote:
> Other than src/test/modules/brin, the ISOLATION users don't look
> much like real extensions (rather than test scaffolding), either.
> If you discount test scaffolding modules then the use-counts are
> more like 4 to 1.

Out of core, the only thing I can see with isolation tests is rum, but
it uses a workaround to have an access to the necessary binaries.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Tom Lane]

Alvaro Herrera  writes:
> I forgot to mention that I considered backpatching this and decided not
> to, but only because it might confuse packagers if they see unrecognized
> files in the installation.  I realize now that c3a0818460a8 was
> back-patched.  Any opinions on backpatching?  

We've added new installed files in minor releases before, true.
But I agree it's something to do only when pretty important, and I'm
not sure this clears the bar.  TAP tests (the facility added by that
other patch) seem way more commonly useful than isolation tests.

Quickly counting the uses in our existing in-core extensions, I see

TAP_TESTS: 3 contrib, 5 src/test/modules
ISOLATION: 1 contrib, 3 src/test/modules

Other than src/test/modules/brin, the ISOLATION users don't look
much like real extensions (rather than test scaffolding), either.
If you discount test scaffolding modules then the use-counts are
more like 4 to 1.

So I'm -0.1 or so on backpatching.

regards, tom lane

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Alvaro Herrera]

On 2020-Oct-15, Alvaro Herrera wrote:

> Pushed, thanks.

I forgot to mention that I considered backpatching this and decided not
to, but only because it might confuse packagers if they see unrecognized
files in the installation.  I realize now that c3a0818460a8 was
back-patched.  Any opinions on backpatching?  

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Alvaro Herrera]

On 2020-Sep-30, Craig Ringer wrote:

> On Tue, 29 Sep 2020 at 22:09, Alvaro Herrera 
> wrote:

> > I happened to come across this thread by accident, and I tend to agree
> > that we need to install both isolationtester and pg_isolation_regress to
> > the pgxs dirs, just like we do pg_regress.  I can't find that
> > isolationtester is installed anywhere though; maybe that changed after
> > you posted this.  Anyway, this version of the patch installs both.

> I think rules were added to allow the isolation tester to be installed with
> an explicit make -C src/test/isolation install, but not added to the
> default "install" target.
> 
> I agree it should just be installed.

Pushed, thanks.

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Craig Ringer]

On Tue, 29 Sep 2020 at 22:09, Alvaro Herrera 
wrote:

> On 2018-Jun-01, Craig Ringer wrote:
>
> > On 28 May 2018 at 15:06, Craig Ringer  wrote:
> >
> > > Per topic, the Pg makefiles install pg_regress (for use by extensions)
> and
> > > htey install the isolationtester, but they don't install
> > > pg_isolation_regress.
> > >
> > > We should install it too.
>
> I happened to come across this thread by accident, and I tend to agree
> that we need to install both isolationtester and pg_isolation_regress to
> the pgxs dirs, just like we do pg_regress.  I can't find that
> isolationtester is installed anywhere though; maybe that changed after
> you posted this.  Anyway, this version of the patch installs both.
>

Thanks.

I think rules were added to allow the isolation tester to be installed with
an explicit make -C src/test/isolation install, but not added to the
default "install" target.

I agree it should just be installed.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Alvaro Herrera]

On 2018-Jun-01, Craig Ringer wrote:

> On 28 May 2018 at 15:06, Craig Ringer  wrote:
> 
> > Per topic, the Pg makefiles install pg_regress (for use by extensions) and
> > htey install the isolationtester, but they don't install
> > pg_isolation_regress.
> >
> > We should install it too.

I happened to come across this thread by accident, and I tend to agree
that we need to install both isolationtester and pg_isolation_regress to
the pgxs dirs, just like we do pg_regress.  I can't find that
isolationtester is installed anywhere though; maybe that changed after
you posted this.  Anyway, this version of the patch installs both.

I did search for evidence in the Makefile's git log that would remove a
recipe for installing isolationtester; couldn't find anything.

> I'm wondering if I should add ISOLATION support to PGXS too, like we have
> REGRESS .

This was covered by Michael's d3c09b9b1307 a few months after you
posted.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 8c81d47ee10bee52b53c8092d8724ce39fed3dfa Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 1 Jun 2018 11:26:09 +0800
Subject: [PATCH v3] Install pg_isolation_regress and isolationtester

---
 src/test/isolation/Makefile | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index da5e088bdd..d23e2cec64 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -18,12 +18,16 @@ OBJS = \
 
 all: isolationtester$(X) pg_isolation_regress$(X)
 
-# Though we don't install these binaries, build them during installation
-# (including temp-install).  Otherwise, "make -j check-world" and "make -j
-# installcheck-world" would spawn multiple, concurrent builds in this
-# directory.  Later builds would overwrite files while earlier builds are
-# reading them, causing occasional failures.
-install: | all
+install: all installdirs
+	$(INSTALL_PROGRAM) pg_isolation_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_isolation_regress$(X)'
+	$(INSTALL_PROGRAM) isolationtester$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/isolationtester$(X)'
+
+installdirs:
+	$(MKDIR_P) '$(DESTDIR)$(pgxsdir)/$(subdir)'
+
+uninstall:
+	rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_isolation_regress$(X)'
+	rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/isolationtester$(X)'
 
 submake-regress:
 	$(MAKE) -C $(top_builddir)/src/test/regress pg_regress.o
-- 
2.20.1

Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Craig Ringer]

On 28 May 2018 at 15:06, Craig Ringer  wrote:

> Hi
>
> Per topic, the Pg makefiles install pg_regress (for use by extensions) and
> htey install the isolationtester, but they don't install
> pg_isolation_regress.
>
> We should install it too.
>

Now with a patch that isn't brain-dead.

I'm wondering if I should add ISOLATION support to PGXS too, like we have
REGRESS .

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 819eda0c40617b57a8ddd1b5d4d30de453bc11ec Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 1 Jun 2018 11:26:09 +0800
Subject: [PATCH v2] Install pg_isolation_regress not just isolationtester

---
 src/test/isolation/Makefile | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index c3c8280ea2..911bb5f43a 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -64,3 +64,9 @@ installcheck-prepared-txns: all temp-install
 
 check-prepared-txns: all temp-install
 	$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions
+
+installdirs:
+	$(MKDIR_P) '$(DESTDIR)$(pgxsdir)/$(subdir)'
+
+install: all installdirs
+	$(INSTALL_PROGRAM) pg_isolation_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_isolation_regress$(X)'
-- 
2.14.3

[PATCH] We install pg_regress and isolationtester but not pg_isolation_regress

[Craig Ringer]

Hi

Per topic, the Pg makefiles install pg_regress (for use by extensions) and
htey install the isolationtester, but they don't install
pg_isolation_regress.

We should install it too.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 47857238a47653081e4d88e2ba14f8712349ebbd Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 28 May 2018 15:04:42 +0800
Subject: [PATCH] Install pg_isolation_regress

---
 src/test/regress/GNUmakefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile
index 587de56fb9..c1fe375f46 100644
--- a/src/test/regress/GNUmakefile
+++ b/src/test/regress/GNUmakefile
@@ -48,6 +48,7 @@ $(top_builddir)/src/port/pg_config_paths.h: $(top_builddir)/src/Makefile.global
 
 install: all installdirs
 	$(INSTALL_PROGRAM) pg_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_regress$(X)'
+	$(INSTALL_PROGRAM) pg_isolation_regress$(X) '$(DESTDIR)$(pgxsdir)/$(subdir)/pg_isolation_regress$(X)'
 
 installdirs:
 	$(MKDIR_P) '$(DESTDIR)$(pgxsdir)/$(subdir)'
-- 
2.14.3