Windows: openssl & gssapi dislike each other

I was able to reproduce the gssapi & openssl error on windows. I tried
on PG16 with msvc build system and on PG17 with meson build system.
The error was reproducible when enabling both openssl and gssapi from
the configurations. Turns out that it was due to the conflicting
macros.

"be-secure-openssl.c" tries to prevent this conflict here [1]https://github.com/postgres/postgres/blob/8ba34c698d19450ccae9a5aea59a6d0bc8b75c0e/src/backend/libpq/be-secure-openssl.c#L46. But the
error again appears when gssapi is enabled. The file
"be-secure-openssl.c" fails to compile because it has a similar
scenario as explained here [2]https://github.com/openssl/openssl/issues/10307#issuecomment-964155382. The header libpq.h is indirectly
including libpq-be.h which has a wrong order of including openssl
headers. Header "gssapi.h" indirectly includes "wincrypt.h" and
openssl header should be defined after gssapi includes.

Now this can either be solved by just just undefine the macro defined
by wincrypt.h as done here [3]https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/contrib/sslinfo/sslinfo.c#L29
```
#ifdef X509_NAME
#undef X509_NAME
#endif
```

Or we should rearrange our headers. Openssl header should be at the
bottom (after the gssapi includes).

I am attaching the patch here in which I rearranged the openssl header
in libpq-be.h

[1]: https://github.com/postgres/postgres/blob/8ba34c698d19450ccae9a5aea59a6d0bc8b75c0e/src/backend/libpq/be-secure-openssl.c#L46
[2]: https://github.com/openssl/openssl/issues/10307#issuecomment-964155382
[3]: https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/contrib/sslinfo/sslinfo.c#L29

Thanks
Imran Zaheer
Bitnine

On 2024-06-08 Sa 06:22, Imran Zaheer wrote:

I was able to reproduce the gssapi & openssl error on windows. I tried
on PG16 with msvc build system and on PG17 with meson build system.
The error was reproducible when enabling both openssl and gssapi from
the configurations. Turns out that it was due to the conflicting
macros.

"be-secure-openssl.c" tries to prevent this conflict here [1]. But the
error again appears when gssapi is enabled. The file
"be-secure-openssl.c" fails to compile because it has a similar
scenario as explained here [2]. The header libpq.h is indirectly
including libpq-be.h which has a wrong order of including openssl
headers. Header "gssapi.h" indirectly includes "wincrypt.h" and
openssl header should be defined after gssapi includes.

Now this can either be solved by just just undefine the macro defined
by wincrypt.h as done here [3]
```
#ifdef X509_NAME
#undef X509_NAME
#endif
```

Or we should rearrange our headers. Openssl header should be at the
bottom (after the gssapi includes).

I am attaching the patch here in which I rearranged the openssl header
in libpq-be.h

[1]: https://github.com/postgres/postgres/blob/8ba34c698d19450ccae9a5aea59a6d0bc8b75c0e/src/backend/libpq/be-secure-openssl.c#L46
[2]: https://github.com/openssl/openssl/issues/10307#issuecomment-964155382
[3]: https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/contrib/sslinfo/sslinfo.c#L29

Let's be consistent and use the #undef from [3]. I did find the comment
in sslinfo.c slightly confusing until I understood that this was a
#define clashing with a typedef.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 2024-06-08 Sa 06:22, Imran Zaheer wrote:

Now this can either be solved by just just undefine the macro defined
by wincrypt.h as done here [3]
Or we should rearrange our headers. Openssl header should be at the
bottom (after the gssapi includes).

Let's be consistent and use the #undef from [3].

+1. Depending on header order is not great, especially when you have
to make it depend on an order that is directly contradictory to
project conventions [0]https://wiki.postgresql.org/wiki/Committing_checklist#Policies.

regards, tom lane

[0]: https://wiki.postgresql.org/wiki/Committing_checklist#Policies

Hi

I am submitting two new patches. We can undefine the macro at two locations

1). As be-secure-openssl.c [1]https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/src/backend/libpq/be-secure-openssl.c#L46 was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h

Thanks
Imran Zaheer
Bitnine

[1]: https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/src/backend/libpq/be-secure-openssl.c#L46

Hi

On Sun, 9 Jun 2024 at 08:29, Imran Zaheer <imran.zhir@gmail.com> wrote:

Hi

I am submitting two new patches. We can undefine the macro at two locations

1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h

Thank you for working on this. I can confirm the undef version compiles and
passes tests with 16.3.

--
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
EDB: https://www.enterprisedb.com

On 2024-06-11 Tu 05:19, Dave Page wrote:

Hi

On Sun, 9 Jun 2024 at 08:29, Imran Zaheer <imran.zhir@gmail.com> wrote:

Hi

I am submitting two new patches. We can undefine the macro at two
locations

1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it
understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h

Thank you for working on this. I can confirm the undef version
compiles and passes tests with 16.3.

Thanks for testing.

I think I prefer approach 2, which should also allow us to remove the
#undef in sslinfo.c so we only need to do this in one place.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Tue, 11 Jun 2024 at 12:22, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2024-06-11 Tu 05:19, Dave Page wrote:

Hi

On Sun, 9 Jun 2024 at 08:29, Imran Zaheer <imran.zhir@gmail.com> wrote:

Hi

I am submitting two new patches. We can undefine the macro at two
locations

1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it
understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h

Thank you for working on this. I can confirm the undef version compiles
and passes tests with 16.3.

Thanks for testing.

I think I prefer approach 2, which should also allow us to remove the
#undef in sslinfo.c so we only need to do this in one place.

OK, well that version compiles and passes tests as well :-)

Thanks!

--
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
EDB: https://www.enterprisedb.com

Hi

I removed the macro from the sslinfo.c as suggested by Andrew. Then I
was thinking maybe we can undo some other similar code.

I rearranged the headers to their previous position in
be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
with gssapi and openssl enabled. You can look into the original commits. [1,
2]
Is it ok if we undo the changes from these commits?

I am attaching two new patches.
One with macro guards removed from ssinfo.c.
Second patch will additionally rearrange headers for
be-secure-openssl.c and in fe-secure-openssl.c to their previous
position.

Thanks
Imran Zaheer
Bitnine

[1]: https://github.com/postgres/postgres/commit/1241fcbd7e649414f09f9858ba73e63975dcff64
[2]: https://github.com/postgres/postgres/commit/568620dfd6912351b4127435eca5309f823abde8

Hi,

On 2024-06-13 00:12:51 +0900, Imran Zaheer wrote:

I removed the macro from the sslinfo.c as suggested by Andrew. Then I
was thinking maybe we can undo some other similar code.

What precisely do you mean by that? Just getting rid of the "ordered include"
of openssl headers in {fe,be}-secure-openssl.h?

I rearranged the headers to their previous position in
be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
with gssapi and openssl enabled. You can look into the original commits. [1,
2]
Is it ok if we undo the changes from these commits?

I am attaching two new patches.
One with macro guards removed from ssinfo.c.
Second patch will additionally rearrange headers for
be-secure-openssl.c and in fe-secure-openssl.c to their previous
position.

One thing that concerns me with this is that there are other includes of
gssapi/gssapi.h (e.g. in , which haven't been changed here. ISTM we ought to do apply
the same change to all of those, otherwise we're just waiting for the problem
to re-appear.

I wonder if we should add a src/include/libpq/pg-gssapi.h or such, which could
wrap the entire ifdeferry for gss support. Something like

#ifdef ENABLE_GSS

#if defined(HAVE_GSSAPI_H)
#include <gssapi.h>
#include <gssapi_ext.h>
#else
#include <gssapi/gssapi.h>
#include <gssapi/gssapi_ext.h>
#endif

/*
* On Windows, <wincrypt.h> includes a #define for X509_NAME, which breaks our
* ability to use OpenSSL's version of that symbol if <wincrypt.h> is pulled
* in after <openssl/ssl.h> ... and, at least on some builds, it is. We
* can't reliably fix that by re-ordering #includes, because libpq/libpq-be.h
* #includes <openssl/ssl.h>. Instead, just zap the #define again here.
*/
#ifdef X509_NAME
#undef X509_NAME
#endif

#endif /* ENABLE_GSS */

Which'd allow the various places using gss (libpq-be.h, be-gssapi-common.h,
libpq-int.h) to just include pg-gssapi.h and get all of the above without
redundancy?

Another thing that concerns me about this approach is that it seems to assume
that the only source of such conflicting includes is gssapi. What if some
other header pulls in wincrypt.h? But I can't really see a way out of that...

Greetings,

Andres Freund

On Tue, Jul 9, 2024 at 2:32 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2024-06-13 00:12:51 +0900, Imran Zaheer wrote:

I removed the macro from the sslinfo.c as suggested by Andrew. Then I
was thinking maybe we can undo some other similar code.

What precisely do you mean by that? Just getting rid of the "ordered include"
of openssl headers in {fe,be}-secure-openssl.h?

Hi

I reordered the includes in {fe,be}-secure-openssl.h as they were also placed
there to resolve similar errors and also were contradicting the
project conventions [1]https://wiki.postgresql.org/wiki/Committing_checklist#Policies (3rd last point).
But looks like it's better to not touch those as they were for future proofing.

I rearranged the headers to their previous position in
be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
with gssapi and openssl enabled. You can look into the original commits. [1,
2]
Is it ok if we undo the changes from these commits?

I am attaching two new patches.
One with macro guards removed from ssinfo.c.
Second patch will additionally rearrange headers for
be-secure-openssl.c and in fe-secure-openssl.c to their previous
position.

One thing that concerns me with this is that there are other includes of
gssapi/gssapi.h (e.g. in , which haven't been changed here. ISTM we ought to do apply
the same change to all of those, otherwise we're just waiting for the problem
to re-appear.

Yes this should be better.

I wonder if we should add a src/include/libpq/pg-gssapi.h or such, which could
wrap the entire ifdeferry for gss support. Something like

#ifdef ENABLE_GSS

#if defined(HAVE_GSSAPI_H)
#include <gssapi.h>
#include <gssapi_ext.h>
#else
#include <gssapi/gssapi.h>
#include <gssapi/gssapi_ext.h>
#endif

/*
* On Windows, <wincrypt.h> includes a #define for X509_NAME, which breaks our
* ability to use OpenSSL's version of that symbol if <wincrypt.h> is pulled
* in after <openssl/ssl.h> ... and, at least on some builds, it is. We
* can't reliably fix that by re-ordering #includes, because libpq/libpq-be.h
* #includes <openssl/ssl.h>. Instead, just zap the #define again here.
*/
#ifdef X509_NAME
#undef X509_NAME
#endif

#endif /* ENABLE_GSS */

Which'd allow the various places using gss (libpq-be.h, be-gssapi-common.h,
libpq-int.h) to just include pg-gssapi.h and get all of the above without
redundancy?

Another thing that concerns me about this approach is that it seems to assume
that the only source of such conflicting includes is gssapi. What if some
other header pulls in wincrypt.h? But I can't really see a way out of that...

Greetings,

Andres Freund

Creating src/include/libpq/pg-gssapi.h can be another great way of
handling these includes. I compiled successfully but couldn't do
proper testing as there is something wrong with my windows env.

And you are right, the approach we are going with right now only
assumes that it's due to the
gssapi as the bug also appeared when building with gssapi (openssl &
gssapi build). What if openssl clashes with
some other lib too which indirectly includes wincrypt.h

For now maybe we can do the future proofing for gssapi & openssl includes
and do testing if openssl clashes with some other lib too.

Thanks
Imran Zaheer

[1]: https://wiki.postgresql.org/wiki/Committing_checklist#Policies (3rd last point)
(3rd last point)

On 10 Jul 2024, at 19:06, Imran Zaheer <imran.zhir@gmail.com> wrote:

(Reviving an old thread to give them a chance to finish before v18)

For now maybe we can do the future proofing for gssapi & openssl includes
and do testing if openssl clashes with some other lib too.

Where did this end up, is compilation on Windows with OpenSSL and GSSAPI still
an issue? AFAICT the fixes from this thread are yet to be applied and it would
be nice to have that done before v18 if still needed.

--
Daniel Gustafsson

On Wed, 22 Jan 2025 at 09:17, Daniel Gustafsson <daniel@yesql.se> wrote:

On 10 Jul 2024, at 19:06, Imran Zaheer <imran.zhir@gmail.com> wrote:

(Reviving an old thread to give them a chance to finish before v18)

For now maybe we can do the future proofing for gssapi & openssl includes
and do testing if openssl clashes with some other lib too.

Where did this end up, is compilation on Windows with OpenSSL and GSSAPI
still
an issue? AFAICT the fixes from this thread are yet to be applied and it
would
be nice to have that done before v18 if still needed.

I kid you not, 20 seconds ago I hit send on a message on a pgAdmin thread
where I was complaining this bug was outstanding to someone asking about
GSSAPI support on Windows. That's quite the coincidence.

Anyway, no fix was committed as far as I know. I would suggest it should
be back-patched as well.

--
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
pgEdge: https://www.pgedge.com

On 2025-01-22 We 4:25 AM, Dave Page wrote:

On Wed, 22 Jan 2025 at 09:17, Daniel Gustafsson <daniel@yesql.se> wrote:

On 10 Jul 2024, at 19:06, Imran Zaheer <imran.zhir@gmail.com> wrote:

(Reviving an old thread to give them a chance to finish before v18)

For now maybe we can do the future proofing for gssapi & openssl

includes

and do testing if openssl clashes with some other lib too.

Where did this end up, is compilation on Windows with OpenSSL and
GSSAPI still
an issue?  AFAICT the fixes from this thread are yet to be applied
and it would
be nice to have that done before v18 if still needed.

I kid you not, 20 seconds ago I hit send on a message on a pgAdmin
thread where I was complaining this bug was outstanding to someone
asking about GSSAPI support on Windows. That's quite the coincidence.

 Anyway, no fix was committed as far as I know. I would suggest it
should be back-patched as well.

I'm quite partial to the approach suggested upthread by Andres (a
separate pg_gssapi.h file). If there's agreement on that I'm prepared to
go and make it happen, unless Daniel beats me to it. Backpatching also
seems reasonable.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Fri, 24 Jan 2025 at 20:07, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2025-01-22 We 4:25 AM, Dave Page wrote:

On Wed, 22 Jan 2025 at 09:17, Daniel Gustafsson <daniel@yesql.se> wrote:

On 10 Jul 2024, at 19:06, Imran Zaheer <imran.zhir@gmail.com> wrote:

(Reviving an old thread to give them a chance to finish before v18)

For now maybe we can do the future proofing for gssapi & openssl

includes

and do testing if openssl clashes with some other lib too.

Where did this end up, is compilation on Windows with OpenSSL and GSSAPI
still
an issue? AFAICT the fixes from this thread are yet to be applied and it
would
be nice to have that done before v18 if still needed.

I kid you not, 20 seconds ago I hit send on a message on a pgAdmin thread
where I was complaining this bug was outstanding to someone asking about
GSSAPI support on Windows. That's quite the coincidence.

Anyway, no fix was committed as far as I know. I would suggest it should
be back-patched as well.

I'm quite partial to the approach suggested upthread by Andres (a separate
pg_gssapi.h file). If there's agreement on that I'm prepared to go and make
it happen, unless Daniel beats me to it. Backpatching also seems reasonable

That solution seems quite elegant to me.

Thanks.

—
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
pgEdge: https://www.pgedge.com

On 24 Jan 2025, at 21:07, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-01-22 We 4:25 AM, Dave Page wrote:

Anyway, no fix was committed as far as I know. I would suggest it should be back-patched as well.

I'm quite partial to the approach suggested upthread by Andres (a separate pg_gssapi.h file). If there's agreement on that I'm prepared to go and make it happen, unless Daniel beats me to it. Backpatching also seems reasonable.

Thanks for the reminder, I also agree that Andres' suggestion is the best
option. I hacked up a patch but got distracted by the pgcrypto GUC patch for a
bit. I'll share what I have once I've done a little testing.

--
Daniel Gustafsson

On 24 Jan 2025, at 22:45, Daniel Gustafsson <daniel@yesql.se> wrote:

On 24 Jan 2025, at 21:07, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-01-22 We 4:25 AM, Dave Page wrote:

Anyway, no fix was committed as far as I know. I would suggest it should be back-patched as well.

I'm quite partial to the approach suggested upthread by Andres (a separate pg_gssapi.h file). If there's agreement on that I'm prepared to go and make it happen, unless Daniel beats me to it. Backpatching also seems reasonable.

Thanks for the reminder, I also agree that Andres' suggestion is the best
option. I hacked up a patch but got distracted by the pgcrypto GUC patch for a
bit. I'll share what I have once I've done a little testing.

After another (conference induced) distraction I remembered this thread again
and tested to build/test the patch against a GSSAPI enabled tree. I think this
is along the right lines.

--
Daniel Gustafsson

Hi,

On 2025-01-31 15:54:45 +0100, Daniel Gustafsson wrote:

On 24 Jan 2025, at 22:45, Daniel Gustafsson <daniel@yesql.se> wrote:

On 24 Jan 2025, at 21:07, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2025-01-22 We 4:25 AM, Dave Page wrote:

Anyway, no fix was committed as far as I know. I would suggest it should be back-patched as well.

I'm quite partial to the approach suggested upthread by Andres (a separate pg_gssapi.h file). If there's agreement on that I'm prepared to go and make it happen, unless Daniel beats me to it. Backpatching also seems reasonable.

Thanks for the reminder, I also agree that Andres' suggestion is the best
option. I hacked up a patch but got distracted by the pgcrypto GUC patch for a
bit. I'll share what I have once I've done a little testing.

After another (conference induced) distraction I remembered this thread again
and tested to build/test the patch against a GSSAPI enabled tree. I think this
is along the right lines.

Largely looks good to me, thanks for tackling this!

diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 2f6c29200ba..f6d8f938c83 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -28,11 +28,7 @@
#include <netinet/tcp.h>

#ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include <gssapi.h>
-#else
-#include <gssapi/gssapi.h>
-#endif							/* HAVE_GSSAPI_H */
+#include "libpq/pg-gssapi.h"
#endif							/* ENABLE_GSS */

This #ifdef ENABLE_GSS probably isn't necessary anymore.

Greetings,

Andres Freund

On 31 Jan 2025, at 16:29, Andres Freund <andres@anarazel.de> wrote:

#ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include <gssapi.h>
-#else
-#include <gssapi/gssapi.h>
-#endif /* HAVE_GSSAPI_H */
+#include "libpq/pg-gssapi.h"
#endif /* ENABLE_GSS */

This #ifdef ENABLE_GSS probably isn't necessary anymore.

Yeah, I only left it for code documentation reasons to keep readers from
thinking the ifdef was missing and had to go chase it in the new file. It's
definitely not required though I for sure don't mind removing it if others feel
it's pointless.

--
Daniel Gustafsson

On Fri, 31 Jan 2025 at 21:05, Daniel Gustafsson <daniel@yesql.se> wrote:

On 31 Jan 2025, at 16:29, Andres Freund <andres@anarazel.de> wrote:

#ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include <gssapi.h>
-#else
-#include <gssapi/gssapi.h>
-#endif /* HAVE_GSSAPI_H */
+#include "libpq/pg-gssapi.h"
#endif /* ENABLE_GSS */

This #ifdef ENABLE_GSS probably isn't necessary anymore.

Yeah, I only left it for code documentation reasons to keep readers from
thinking the ifdef was missing and had to go chase it in the new file. It's
definitely not required though I for sure don't mind removing it if others feel
it's pointless.

Few thoughts:
1) I also felt that this could be removed.

2) Was the copyright year retained as 1996 intentionally for the new
"pg-gssapi.h" file added because the contents were copied from other
files?
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California

I see in few other places were new file was created, it was mentioned
as "Copyright (c) 2025, PostgreSQL Global Development Group"

3) Apart from that, there was a small whitespace issue while applying the patch:
git am v1-0001-Move-GSSAPI-includes-into-its-own-header.patch
Applying: Move GSSAPI includes into its own header
.git/rebase-apply/patch:116: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Overall patch looks good to me.

Regards,
Vignesh

On 2025-03-26 We 4:53 AM, vignesh C wrote:

On Fri, 31 Jan 2025 at 21:05, Daniel Gustafsson <daniel@yesql.se> wrote:

On 31 Jan 2025, at 16:29, Andres Freund <andres@anarazel.de> wrote:

#ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include <gssapi.h>
-#else
-#include <gssapi/gssapi.h>
-#endif /* HAVE_GSSAPI_H */
+#include "libpq/pg-gssapi.h"
#endif /* ENABLE_GSS */

This #ifdef ENABLE_GSS probably isn't necessary anymore.

Yeah, I only left it for code documentation reasons to keep readers from
thinking the ifdef was missing and had to go chase it in the new file. It's
definitely not required though I for sure don't mind removing it if others feel
it's pointless.

Few thoughts:
1) I also felt that this could be removed.

2) Was the copyright year retained as 1996 intentionally for the new
"pg-gssapi.h" file added because the contents were copied from other
files?
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California

I see in few other places were new file was created, it was mentioned
as "Copyright (c) 2025, PostgreSQL Global Development Group"

3) Apart from that, there was a small whitespace issue while applying the patch:
git am v1-0001-Move-GSSAPI-includes-into-its-own-header.patch
Applying: Move GSSAPI includes into its own header
.git/rebase-apply/patch:116: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Overall patch looks good to me.

LGTM too.

I was hoping to test it but I haven't been able to set up an environment
for testing. But I don't think that should hold it up.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 26 Mar 2025, at 09:53, vignesh C <vignesh21@gmail.com> wrote:
On Fri, 31 Jan 2025 at 21:05, Daniel Gustafsson <daniel@yesql.se> wrote:

On 31 Jan 2025, at 16:29, Andres Freund <andres@anarazel.de> wrote:

This #ifdef ENABLE_GSS probably isn't necessary anymore.

Yeah, I only left it for code documentation reasons to keep readers from
thinking the ifdef was missing and had to go chase it in the new file. It's
definitely not required though I for sure don't mind removing it if others feel
it's pointless.

Few thoughts:
1) I also felt that this could be removed.

Fixed.

2) Was the copyright year retained as 1996 intentionally for the new
"pg-gssapi.h" file added because the contents were copied from other
files?
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California

I see in few other places were new file was created, it was mentioned
as "Copyright (c) 2025, PostgreSQL Global Development Group"

Right, this is new file but not net-new content so I retained the copyright
from the original location. We are standing on the shoulders of giants.

3) Apart from that, there was a small whitespace issue while applying the patch:
git am v1-0001-Move-GSSAPI-includes-into-its-own-header.patch
Applying: Move GSSAPI includes into its own header
.git/rebase-apply/patch:116: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Fixed.

Overall patch looks good to me.

Thanks for review! Pushed after making the above changes and taking it for
another CI run.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

Thanks for review! Pushed after making the above changes and taking it for
another CI run.

CF entry should be marked closed no?

https://commitfest.postgresql.org/patch/5060/

regards, tom lane

On 26 Mar 2025, at 17:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

Thanks for review! Pushed after making the above changes and taking it for
another CI run.

CF entry should be marked closed no?

Yep, just wanted to allow some time to see how the buildfarm liked it before
closing. Done now, thanks for the reminder.

--
Daniel Gustafsson

On Wed, 26 Mar 2025 at 16:32, Daniel Gustafsson <daniel@yesql.se> wrote:

On 26 Mar 2025, at 17:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

Thanks for review! Pushed after making the above changes and taking it

for

another CI run.

CF entry should be marked closed no?

Yep, just wanted to allow some time to see how the buildfarm liked it
before
closing. Done now, thanks for the reminder.

Thank you for dealing with this!

--
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
pgEdge: https://www.pgedge.com