Re: Reduce the number of special cases to build contrib modules on windows

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Reduce the number of special cases to build contrib modules on windows
Date: 2020-12-23 05:46:19
Message-ID: X+LZq7J9RH4ddpmK@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Dec 22, 2020 at 11:24:40PM +1300, David Rowley wrote:
> On Wed, 11 Nov 2020 at 13:44, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> It seems to me that your patch is doing the right thing for adminpack
>> and that its Makefile has no need to include a reference to libpq
>> source path, no?
>
> Yeah. Likely a separate commit should remove the -I$(libpq_srcdir)
> from adminpack and old_snapshot

I have begun a new thread about this point as that's a separate
topic. I did not see other places in need of a similar cleanup:
https://www.postgresql.org/message-id/X+LQpfLyk7jgzUki@paquier.xyz

> I didn't look in detail, but it looks like if we define LOWER_NODE on
> Windows that it might break pg_upgrade. I guess you could say it's
> partially broken now as the behaviour there will depend on if you
> build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin
> but not on VS. Looks like a pg_upgrade might be problematic there
> today.
>
> It feels a bit annoying to add some special case to the script to
> maintain the status quo there. An alternative to that would be to
> modify the .c code at #ifdef LOWER_NODE to also check we're not
> building on VS. Neither option seems nice.

Hmm. It seems that you are right here. This influences lquery
parsing so it may be nasty and this exists since ltree is present in
the tree (2002). I think that I would choose the update in the C code
and remove LOWER_NODE while keeping the scripts clean, and documenting
directly in the code why this compatibility issue exists.
REFINT_VERBOSE is no problem, fortunately.

> I've attached the updated patch and also a diff showing the changes in
> the *.vcxproj files.

Thanks!

> There are quite a few places where the hash table code for includes
> and references gets rid of duplicates that already exist today. For
> example pgbench.vcxproj references libpgport.vcxproj and
> libpgcommon.vcxproj twice.

The diffs look clean. dblink has lost src/backend/, there are the
additions of REFINT_VERBOSE and LOWER_NODE but the bulk of the diffs
comes from a change in the order of items listed, while removing
duplicates.

I have tested your patch, and this is causing compilation failures for
hstore_plpython, jsonb_plpython and ltree_plpython. So
AddTransformModule is missing something here when compiling with
Python.
--
Michael

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Zhihong Yu 2020-12-23 05:51:59 Re: [Patch] Optimize dropping of relation buffers using dlist
Previous Message Justin Pryzby 2020-12-23 05:22:00 Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly