Horton

Thread

Commits

  1. Fix most -Wundef warnings

  2. Remove some code for old unsupported versions of MSVC

  1. Fix most -Wundef warnings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-10-13T19:25:59Z

    During the cleanup of the _MSC_VER versions (commit
38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
-Wundef, but that resulted in a bunch of gratuitous warnings.  Here is a
patch to fix those.  Most of these are just stylistic cleanups, but the
change in pg_bswap.h is potentially useful to avoid misuse by
third-party extensions.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: Fix most -Wundef warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-13T19:56:59Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> During the cleanup of the _MSC_VER versions (commit
> 38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
> -Wundef, but that resulted in a bunch of gratuitous warnings.  Here is a
> patch to fix those.  Most of these are just stylistic cleanups, but the
> change in pg_bswap.h is potentially useful to avoid misuse by
> third-party extensions.

Looks reasonable offhand.

			regards, tom lane

  3. Re: Fix most -Wundef warnings

    Mark Dilger <hornschnorter@gmail.com> — 2019-10-14T15:12:29Z

    
On 10/13/19 12:25 PM, Peter Eisentraut wrote:
> diff --git a/contrib/hstore/hstore_compat.c b/contrib/hstore/hstore_compat.c
> index 1d4e7484e4..d75e9cb23f 100644
> --- a/contrib/hstore/hstore_compat.c
> +++ b/contrib/hstore/hstore_compat.c
> @@ -299,7 +299,7 @@ hstoreUpgrade(Datum orig)
>   
>   	if (valid_new)
>   	{
> -#if HSTORE_IS_HSTORE_NEW
> +#ifdef HSTORE_IS_HSTORE_NEW
>   		elog(WARNING, "ambiguous hstore value resolved as hstore-new");

Checking the current sources, git history, and various older commits, I 
did not find where HSTORE_IS_HSTORE_NEW was ever defined.  I expect it 
was defined at some point, but I checked back as far as 9.0 (where the 
current contrib/hstore was originally committed) and did not see it. 
Where did you find this, and can we add a code comment?  This one #ifdef 
is the only line in the entire repository where this label is used, 
making it hard to check if changing from #if was the right decision.

The check on HSTORE_IS_HSTORE_NEW goes back at least as far as 2006, 
suggesting it was needed for migrating from some version pre-9.0, making 
me wonder if anybody would need this in the field.  Should we drop 
support for this?  I don't have a strong reason to advocate dropping 
support other than that this #define appears to be undocumented.

mark

  4. Re: Fix most -Wundef warnings

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-10-15T12:23:43Z

    >>>>> "Mark" == Mark Dilger <hornschnorter@gmail.com> writes:

 >> +#ifdef HSTORE_IS_HSTORE_NEW

 Mark> Checking the current sources, git history, and various older
 Mark> commits, I did not find where HSTORE_IS_HSTORE_NEW was ever
 Mark> defined.

In contrib/hstore, it never was.

The current version of contrib/hstore had a brief life as a separate
extension module called hstore-new, which existed to backport its
functionality into 8.4. The data format for hstore-new was almost
identical to the new contrib/hstore one (and thus different from the old
contrib/hstore), and changed at one point before its final release, so
there were four possible upgrade paths as explained in the comments.

The block comment with the most pertinent explanation seems to have
been a victim of pgindent, but the relevant part is this:

 * [...] So the upshot of all this
 * is that we can treat all the edge cases as "new" if we're being built
 * as hstore-new, and "old" if we're being built as contrib/hstore.

So, HSTORE_IS_HSTORE_NEW was defined if you were building a pgxs module
called "hstore-new" (which was distributed separately on pgfoundry but
the C code was the same), and not if you're building "hstore" (whether
an in or out of tree build).

 Mark> The check on HSTORE_IS_HSTORE_NEW goes back at least as far as
 Mark> 2006, suggesting it was needed for migrating from some version
 Mark> pre-9.0, making me wonder if anybody would need this in the
 Mark> field. Should we drop support for this? I don't have a strong
 Mark> reason to advocate dropping support other than that this #define
 Mark> appears to be undocumented.

The only reason not to remove most of hstore_compat.c is that there is
no way to know what data survives in the wild in each of the three
possible hstore formats (8.4 contrib, pre-final hstore-new, current). I
think it's most unlikely that any of the pre-final hstore-new data still
exists, but how would anyone know?

(The fact that there have been exactly zero field reports of either of
the WARNING messages unfortunately doesn't prove much. Almost all
possible non-current hstore values are unambiguously in one or other of
the possible formats, the ambiguity is only possible because the old
code didn't always set the varlena length to the correct size, but left
unused space at the end.)

-- 
Andrew (irc:RhodiumToad)

  5. Re: Fix most -Wundef warnings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-10-19T16:50:10Z

    On 2019-10-13 21:25, Peter Eisentraut wrote:
> During the cleanup of the _MSC_VER versions (commit
> 38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
> -Wundef, but that resulted in a bunch of gratuitous warnings.  Here is a
> patch to fix those.  Most of these are just stylistic cleanups, but the
> change in pg_bswap.h is potentially useful to avoid misuse by
> third-party extensions.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Fix most -Wundef warnings

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-10-19T16:51:33Z

    On 2019-10-14 17:12, Mark Dilger wrote:
> The check on HSTORE_IS_HSTORE_NEW goes back at least as far as 2006, 
> suggesting it was needed for migrating from some version pre-9.0, making 
> me wonder if anybody would need this in the field.  Should we drop 
> support for this?  I don't have a strong reason to advocate dropping 
> support other than that this #define appears to be undocumented.

Per subsequent messages in this thread, this issue is outside the scope
of my patch, so I proceeded with my patch as I had proposed it.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: Fix most -Wundef warnings

    Mark Dilger <hornschnorter@gmail.com> — 2019-10-21T03:02:18Z

    
On 10/15/19 5:23 AM, Andrew Gierth wrote:
>>>>>> "Mark" == Mark Dilger <hornschnorter@gmail.com> writes:
> 
>   >> +#ifdef HSTORE_IS_HSTORE_NEW
> 
>   Mark> Checking the current sources, git history, and various older
>   Mark> commits, I did not find where HSTORE_IS_HSTORE_NEW was ever
>   Mark> defined.
> 
> In contrib/hstore, it never was.
> 
> The current version of contrib/hstore had a brief life as a separate
> extension module called hstore-new, which existed to backport its
> functionality into 8.4. The data format for hstore-new was almost
> identical to the new contrib/hstore one (and thus different from the old
> contrib/hstore), and changed at one point before its final release, so
> there were four possible upgrade paths as explained in the comments.
> 
> The block comment with the most pertinent explanation seems to have
> been a victim of pgindent, but the relevant part is this:
> 
>   * [...] So the upshot of all this
>   * is that we can treat all the edge cases as "new" if we're being built
>   * as hstore-new, and "old" if we're being built as contrib/hstore.
> 
> So, HSTORE_IS_HSTORE_NEW was defined if you were building a pgxs module
> called "hstore-new" (which was distributed separately on pgfoundry but
> the C code was the same), and not if you're building "hstore" (whether
> an in or out of tree build).

I don't really dispute your claim, but it doesn't unambiguously follow 
from the wording of the comment.  The part that tripped me up while 
reviewing Peter's patch is that he changed the preprocessor logic to use 
#ifdef rather than #if, implying that he believes HSTORE_IS_HSTORE_NEW 
will only be defined when true, and undefined when false, rather than 
something like:

   #if OLD_STUFF
   #define HSTORE_IS_HSTORE_NEW 0
   #else
   #define HSTORE_IS_HSTORE_NEW 1
   #endif

which is admittedly a less common coding pattern than only defining it 
when true, but the choice of #if rather than #ifdef in the original 
sources might have been intentional.

I tried briefly to download this project from pgfoundry without success. 
  Do you have a copy of the relevant code where you can see how this 
gets defined, and can you include it in a reply?

Thanks,

mark

  8. Re: Fix most -Wundef warnings

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-10-21T04:38:06Z

    >>>>> "Mark" == Mark Dilger <hornschnorter@gmail.com> writes:

 Mark> I tried briefly to download this project from pgfoundry without
 Mark> success. Do you have a copy of the relevant code where you can
 Mark> see how this gets defined, and can you include it in a reply?

I have a backup of the CVS from the pgfoundry version, but the thing is
so obsolete that I had never bothered converting it to git; it hasn't
been touched in 10 years.

The Makefile had this:

PG_CPPFLAGS = -DHSTORE_IS_HSTORE_NEW

The only possible use for this code is if someone were to discover an
old 8.4 install with an old hstore-new module in use. I think the
chances of this are small enough not to be of much concern.

I have put up a CVS->Git conversion for the benefit of software
archaeologists only at: https://github.com/RhodiumToad/hstore-ancient

-- 
Andrew (irc:RhodiumToad)