public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3 4/4] Use hidden visibility for shared libraries where possible. 7+ messages / 5 participants [nested] [flat]
* [PATCH v3 4/4] Use hidden visibility for shared libraries where possible. @ 2022-07-16 20:08 Andres Freund <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Andres Freund @ 2022-07-16 20:08 UTC (permalink / raw) Author: Andres Freund <[email protected]> Author: Tom Lane <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/include/c.h | 13 +++- src/include/pg_config.h.in | 3 + src/makefiles/pgxs.mk | 5 +- configure | 152 +++++++++++++++++++++++++++++++++++++ configure.ac | 13 ++++ src/Makefile.global.in | 2 + src/Makefile.shlib | 13 ++++ src/tools/msvc/Project.pm | 7 -- src/tools/msvc/Solution.pm | 1 + 9 files changed, 197 insertions(+), 12 deletions(-) diff --git a/src/include/c.h b/src/include/c.h index 863a16c6a6c..2cc2784750e 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -1347,14 +1347,19 @@ extern unsigned long long strtoull(const char *str, char **endptr, int base); /* * Use "extern PGDLLEXPORT ..." to declare functions that are defined in - * loadable modules and need to be callable by the core backend. (Usually, - * this is not necessary because our build process automatically exports - * such symbols, but sometimes manual marking is required.) - * No special marking is required on most ports. + * loadable modules and need to be callable by the core backend or other + * loadable modules. + * If the compiler knows __attribute__((visibility("*"))), we use that, + * unless we already have a platform-specific definition. Otherwise, + * no special marking is required. */ #ifndef PGDLLEXPORT +#ifdef HAVE_VISIBILITY_ATTRIBUTE +#define PGDLLEXPORT __attribute__((visibility("default"))) +#else #define PGDLLEXPORT #endif +#endif /* * The following is used as the arg list for signal handlers. Any ports diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 7133c3dc66b..529fb84a86c 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -700,6 +700,9 @@ /* Define to 1 if you have the <uuid/uuid.h> header file. */ #undef HAVE_UUID_UUID_H +/* Define to 1 if your compiler knows the visibility("hidden") attribute. */ +#undef HAVE_VISIBILITY_ATTRIBUTE + /* Define to 1 if you have the `wcstombs_l' function. */ #undef HAVE_WCSTOMBS_L diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index 0f71fa293d0..cc62df4793b 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -101,8 +101,11 @@ endif # PGXS override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS) +# See equivalent block in Makefile.shlib ifdef MODULES -override CFLAGS += $(CFLAGS_SL) +override LDFLAGS_SL += $(CFLAGS_SL_MOD) +override CFLAGS += $(CFLAGS_SL) $(CFLAGS_SL_MOD) +override CXXFLAGS += $(CFLAGS_SL) $(CXXFLAGS_SL_MOD) endif ifdef MODULEDIR diff --git a/configure b/configure index 1e63c6862bc..3b752941a0c 100755 --- a/configure +++ b/configure @@ -741,6 +741,8 @@ CPP CFLAGS_SL BITCODE_CXXFLAGS BITCODE_CFLAGS +CXXFLAGS_SL_MOD +CFLAGS_SL_MOD CFLAGS_VECTORIZE CFLAGS_UNROLL_LOOPS PERMIT_DECLARATION_AFTER_STATEMENT @@ -6302,6 +6304,154 @@ if test x"$pgac_cv_prog_CC_cflags__ftree_vectorize" = x"yes"; then fi + # + # If the compiler knows how to hide symbols, set CFLAGS_SL_MOD + # to the switch needed for that, and define HAVE_VISIBILITY_ATTRIBUTE. + { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -fvisibility=hidden, for CFLAGS_SL_MOD" >&5 +$as_echo_n "checking whether ${CC} supports -fvisibility=hidden, for CFLAGS_SL_MOD... " >&6; } +if ${pgac_cv_prog_CC_cflags__fvisibility_hidden+:} false; then : + $as_echo_n "(cached) " >&6 +else + pgac_save_CFLAGS=$CFLAGS +pgac_save_CC=$CC +CC=${CC} +CFLAGS="${CFLAGS_SL_MOD} -fvisibility=hidden" +ac_save_c_werror_flag=$ac_c_werror_flag +ac_c_werror_flag=yes +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +int +main () +{ + + ; + return 0; +} +_ACEOF +if ac_fn_c_try_compile "$LINENO"; then : + pgac_cv_prog_CC_cflags__fvisibility_hidden=yes +else + pgac_cv_prog_CC_cflags__fvisibility_hidden=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +ac_c_werror_flag=$ac_save_c_werror_flag +CFLAGS="$pgac_save_CFLAGS" +CC="$pgac_save_CC" +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__fvisibility_hidden" >&5 +$as_echo "$pgac_cv_prog_CC_cflags__fvisibility_hidden" >&6; } +if test x"$pgac_cv_prog_CC_cflags__fvisibility_hidden" = x"yes"; then + CFLAGS_SL_MOD="${CFLAGS_SL_MOD} -fvisibility=hidden" +fi + + + if test "$pgac_cv_prog_CC_cflags__fvisibility_hidden" = yes; then + +$as_echo "#define HAVE_VISIBILITY_ATTRIBUTE 1" >>confdefs.h + + fi + # For C++ we additionally want -fvisibility-inlines-hidden + { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -fvisibility=hidden, for CXXFLAGS_SL_MOD" >&5 +$as_echo_n "checking whether ${CXX} supports -fvisibility=hidden, for CXXFLAGS_SL_MOD... " >&6; } +if ${pgac_cv_prog_CXX_cxxflags__fvisibility_hidden+:} false; then : + $as_echo_n "(cached) " >&6 +else + pgac_save_CXXFLAGS=$CXXFLAGS +pgac_save_CXX=$CXX +CXX=${CXX} +CXXFLAGS="${CXXFLAGS_SL_MOD} -fvisibility=hidden" +ac_save_cxx_werror_flag=$ac_cxx_werror_flag +ac_cxx_werror_flag=yes +ac_ext=cpp +ac_cpp='$CXXCPP $CPPFLAGS' +ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_cxx_compiler_gnu + +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +int +main () +{ + + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_compile "$LINENO"; then : + pgac_cv_prog_CXX_cxxflags__fvisibility_hidden=yes +else + pgac_cv_prog_CXX_cxxflags__fvisibility_hidden=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +ac_ext=c +ac_cpp='$CPP $CPPFLAGS' +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_c_compiler_gnu + +ac_cxx_werror_flag=$ac_save_cxx_werror_flag +CXXFLAGS="$pgac_save_CXXFLAGS" +CXX="$pgac_save_CXX" +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" >&5 +$as_echo "$pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" >&6; } +if test x"$pgac_cv_prog_CXX_cxxflags__fvisibility_hidden" = x"yes"; then + CXXFLAGS_SL_MOD="${CXXFLAGS_SL_MOD} -fvisibility=hidden" +fi + + { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -fvisibility-inlines-hidden, for CXXFLAGS_SL_MOD" >&5 +$as_echo_n "checking whether ${CXX} supports -fvisibility-inlines-hidden, for CXXFLAGS_SL_MOD... " >&6; } +if ${pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden+:} false; then : + $as_echo_n "(cached) " >&6 +else + pgac_save_CXXFLAGS=$CXXFLAGS +pgac_save_CXX=$CXX +CXX=${CXX} +CXXFLAGS="${CXXFLAGS_SL_MOD} -fvisibility-inlines-hidden" +ac_save_cxx_werror_flag=$ac_cxx_werror_flag +ac_cxx_werror_flag=yes +ac_ext=cpp +ac_cpp='$CXXCPP $CPPFLAGS' +ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_cxx_compiler_gnu + +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ + +int +main () +{ + + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_compile "$LINENO"; then : + pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden=yes +else + pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +ac_ext=c +ac_cpp='$CPP $CPPFLAGS' +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_c_compiler_gnu + +ac_cxx_werror_flag=$ac_save_cxx_werror_flag +CXXFLAGS="$pgac_save_CXXFLAGS" +CXX="$pgac_save_CXX" +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" >&5 +$as_echo "$pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" >&6; } +if test x"$pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" = x"yes"; then + CXXFLAGS_SL_MOD="${CXXFLAGS_SL_MOD} -fvisibility-inlines-hidden" +fi + # # The following tests want to suppress various unhelpful warnings by adding # -Wno-foo switches. But gcc won't complain about unrecognized -Wno-foo @@ -6860,6 +7010,8 @@ fi + + # Determine flags used to emit bitcode for JIT inlining. # 1. We must duplicate any behaviour-changing compiler flags used above, # to keep compatibility with the compiler used for normal Postgres code. diff --git a/configure.ac b/configure.ac index 71191f14ad7..478da1971e7 100644 --- a/configure.ac +++ b/configure.ac @@ -525,6 +525,17 @@ if test "$GCC" = yes -a "$ICC" = no; then # Optimization flags for specific files that benefit from vectorization PGAC_PROG_CC_VAR_OPT(CFLAGS_VECTORIZE, [-ftree-vectorize]) # + # If the compiler knows how to hide symbols, set CFLAGS_SL_MOD + # to the switch needed for that, and define HAVE_VISIBILITY_ATTRIBUTE. + PGAC_PROG_CC_VAR_OPT(CFLAGS_SL_MOD, [-fvisibility=hidden]) + if test "$pgac_cv_prog_CC_cflags__fvisibility_hidden" = yes; then + AC_DEFINE([HAVE_VISIBILITY_ATTRIBUTE], 1, + [Define to 1 if your compiler knows the visibility("hidden") attribute.]) + fi + # For C++ we additionally want -fvisibility-inlines-hidden + PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MOD, [-fvisibility=hidden]) + PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MOD, [-fvisibility-inlines-hidden]) + # # The following tests want to suppress various unhelpful warnings by adding # -Wno-foo switches. But gcc won't complain about unrecognized -Wno-foo # switches, so we have to test for the positive form and if that works, @@ -573,6 +584,8 @@ fi AC_SUBST(CFLAGS_UNROLL_LOOPS) AC_SUBST(CFLAGS_VECTORIZE) +AC_SUBST(CFLAGS_SL_MOD) +AC_SUBST(CXXFLAGS_SL_MOD) # Determine flags used to emit bitcode for JIT inlining. # 1. We must duplicate any behaviour-changing compiler flags used above, diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 138d66ac006..2d58f1d7f49 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -258,6 +258,8 @@ SUN_STUDIO_CC = @SUN_STUDIO_CC@ CXX = @CXX@ CFLAGS = @CFLAGS@ CFLAGS_SL = @CFLAGS_SL@ +CFLAGS_SL_MOD = @CFLAGS_SL_MOD@ +CXXFLAGS_SL_MOD = @CXXFLAGS_SL_MOD@ CFLAGS_UNROLL_LOOPS = @CFLAGS_UNROLL_LOOPS@ CFLAGS_VECTORIZE = @CFLAGS_VECTORIZE@ CFLAGS_SSE42 = @CFLAGS_SSE42@ diff --git a/src/Makefile.shlib b/src/Makefile.shlib index 6df96c634b6..5ec8209cc79 100644 --- a/src/Makefile.shlib +++ b/src/Makefile.shlib @@ -218,6 +218,19 @@ ifeq ($(PORTNAME), win32) endif +# If the shared library doesn't have an export file, mark all symbols not +# explicitly exported using PGDLLEXPORT as hidden. We can't pass these flags +# when building a library with explicit exports, as the symbols would be +# hidden before the linker script / exported symbol list takes effect. +# +# This is duplicated in pgxs.mk for MODULES style libraries. +ifeq ($(SHLIB_EXPORTS),) + # LDFLAGS_SL addition not strictly needed, CFLAGS used everywhere, but ... + override LDFLAGS_SL += $(CFLAGS_SL_MOD) + override CFLAGS += $(CFLAGS_SL_MOD) + override CXXFLAGS += $(CXXFLAGS_SL_MOD) +endif + ## ## BUILD diff --git a/src/tools/msvc/Project.pm b/src/tools/msvc/Project.pm index 570bab563a7..b24a2a98155 100644 --- a/src/tools/msvc/Project.pm +++ b/src/tools/msvc/Project.pm @@ -419,13 +419,6 @@ sub Save { my ($self) = @_; - # If doing DLL and haven't specified a DEF file, do a full export of all symbols - # in the project. - if ($self->{type} eq "dll" && !$self->{def}) - { - $self->FullExportDLL($self->{name} . ".lib"); - } - # Warning 4197 is about double exporting, disable this per # http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=99193 $self->DisableLinkerWarnings('4197') if ($self->{platform} eq 'x64'); diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index fa32dc371dc..f2427008df6 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -429,6 +429,7 @@ sub GenerateFiles HAVE_WINLDAP_H => undef, HAVE_WCSTOMBS_L => 1, HAVE_WCTYPE_H => 1, + HAVE_VISIBILITY_ATTRIBUTE => undef, HAVE_WRITEV => undef, HAVE_X509_GET_SIGNATURE_NID => 1, HAVE_X86_64_POPCNTQ => undef, -- 2.37.0.3.g30cc8d0f14 --p7jx4oxz2gonjjqa-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-10 19:30 Isaac Morland <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Isaac Morland @ 2025-02-10 19:30 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Amul Sul <[email protected]>; Peter Eisentraut <[email protected]>; Ashutosh Bapat <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On Mon, 10 Feb 2025 at 13:48, Álvaro Herrera <[email protected]> wrote: I think this proposed state of affairs is problematic. Current queries > that assume that pg_constraint.convalidated means that a constraint is > validated would be broken. My suggestion at this point is that instead of > adding a separate boolean column to pg_constraint we should be replacing > `bool convalidated` with `char convalidity`, with new defines for all the > possible states we require: enforced-and-valid ("V"alid), > enforced-not-validated ("i"nvalid), not-enforced-and-not-valid (terribly > "I"nvalid or maybe "U"nenforced), > not-enforced-but-was-valid-before-turning-unenforced ("u"nenforced). > Breaking user queries would make all apps reassess what do they actually > want to know about the constraint without assumptions of how enforcement > worked in existing Postgres releases. > I'm having a lot of trouble understanding the operational distinction between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be valid, regardless of whether it was valid in the past. I'm not sure what I think of a single character vs. 2 booleans, but there are only 3 sensible states either way: valid enforced, invalid enforced, and invalid unenforced. Additionally, if there are officially 4 status possibilities then all code that looks for unenforced constraints has to look for both valid and invalid unenforced constraints if we use a char; it's not as bad with 2 booleans because one can just check the "enforced" boolean. ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-11 13:36 Álvaro Herrera <[email protected]> parent: Isaac Morland <[email protected]> 0 siblings, 2 replies; 7+ messages in thread From: Álvaro Herrera @ 2025-02-11 13:36 UTC (permalink / raw) To: Isaac Morland <[email protected]>; +Cc: Amul Sul <[email protected]>; Peter Eisentraut <[email protected]>; Ashutosh Bapat <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On 2025-Feb-10, Isaac Morland wrote: > I'm having a lot of trouble understanding the operational distinction > between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be > valid, regardless of whether it was valid in the past. I'm not sure what I > think of a single character vs. 2 booleans, but there are only 3 sensible > states either way: valid enforced, invalid enforced, and invalid unenforced. I kinda agree with you and would prefer that things were that way as well. But look at the discussion starting at https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com whereby it was apparently established that if you have a NOT VALID NOT ENFORCED constraint, and you make it enforced, then you should somehow end up with a NOT VALID ENFORCED constraint, which says to me that we need to store the fact that the constraint was NOT VALID to start with; and correspondingly if it's VALID NOT ENFORCED and you enforce it, then it ends up VALID ENFORCED. If we take this view of the world (with which, I repeat, I disagree) then we must keep track of whether the constraint was valid or not valid to start with. And this means that we need to keep convalidated=true _regardless_ of whether conenforced is false. So in this view of the world there aren't three states but four. I would prefer there to be three states as well, but apparently I'm outvoted on this. > Additionally, if there are officially 4 status possibilities then all code > that looks for unenforced constraints has to look for both valid and > invalid unenforced constraints if we use a char; it's not as bad with 2 > booleans because one can just check the "enforced" boolean. Well, yes. You have kinda the same issue with any other system catalog column that's a 'char', I guess. Maybe this is more of a problem here because it's more user-visible than most other catalogs, not sure. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-11 15:39 Isaac Morland <[email protected]> parent: Álvaro Herrera <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Isaac Morland @ 2025-02-11 15:39 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Amul Sul <[email protected]>; Peter Eisentraut <[email protected]>; Ashutosh Bapat <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <[email protected]> wrote: > On 2025-Feb-10, Isaac Morland wrote: > > > I'm having a lot of trouble understanding the operational distinction > > between your 'u' and 'U'. If it's not enforced, it cannot be assumed to > be > > valid, regardless of whether it was valid in the past. I'm not sure what > I > > think of a single character vs. 2 booleans, but there are only 3 sensible > > states either way: valid enforced, invalid enforced, and invalid > unenforced. > > I kinda agree with you and would prefer that things were that way as > well. But look at the discussion starting at > > https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com > whereby it was apparently established that if you have a > NOT VALID NOT ENFORCED > constraint, and you make it enforced, then you should somehow end up > with a NOT VALID ENFORCED constraint, which says to me that we need to > store the fact that the constraint was NOT VALID to start with; and > correspondingly if it's VALID NOT ENFORCED and you enforce it, then it > ends up VALID ENFORCED. If we take this view of the world (with which, > I repeat, I disagree) then we must keep track of whether the constraint > was valid or not valid to start with. And this means that we need to > keep convalidated=true _regardless_ of whether conenforced is false. > So in this view of the world there aren't three states but four. > > I would prefer there to be three states as well, but apparently I'm > outvoted on this. > Sounds like we agree. I think the problem is with the statement in the linked discussion that “If the constraint is VALID and later marked as NOT ENFORCED, changing it to ENFORCED should also keep it VALID.” This ignores that if it is changed to NOT ENFORCED that should immediately change it to NOT VALID if it is not already so. Has anybody argued for how it makes any sense at all to have a constraint that is VALID (and therefore will be assumed to be true by the planner), yet NOT ENFORCED (and therefore may well not be true)? What next, a patch to the planner so that it only treats as true constraints that are both VALID and ENFORCED? Re: the 3 or 4 values for the single character status, there is a similar issue with relkind, where one can imagine writing "relkind IN ('r')" when one meant "relkind IN ('r', 'v')" or something else; but on the other hand, one can easily imagine actually wanting the first one of those. But here, it's not at all clear to me when you would ever want to distinguish between 'u' and 'U', but it is clear to me that it would be natural to write "… = 'U'" when one actually needs to write "… IN ('u', 'U')", or perhaps "… ILIKE 'u'" (not what I would want to see). ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-12 04:47 Ashutosh Bapat <[email protected]> parent: Isaac Morland <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Ashutosh Bapat @ 2025-02-12 04:47 UTC (permalink / raw) To: Isaac Morland <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Amul Sul <[email protected]>; Peter Eisentraut <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On Tue, Feb 11, 2025 at 9:09 PM Isaac Morland <[email protected]> wrote: > > On Tue, 11 Feb 2025 at 08:36, Álvaro Herrera <[email protected]> wrote: >> >> On 2025-Feb-10, Isaac Morland wrote: >> >> > I'm having a lot of trouble understanding the operational distinction >> > between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be >> > valid, regardless of whether it was valid in the past. I'm not sure what I >> > think of a single character vs. 2 booleans, but there are only 3 sensible >> > states either way: valid enforced, invalid enforced, and invalid unenforced. >> >> I kinda agree with you and would prefer that things were that way as >> well. But look at the discussion starting at >> https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com >> whereby it was apparently established that if you have a >> NOT VALID NOT ENFORCED >> constraint, and you make it enforced, then you should somehow end up >> with a NOT VALID ENFORCED constraint, which says to me that we need to >> store the fact that the constraint was NOT VALID to start with; and >> correspondingly if it's VALID NOT ENFORCED and you enforce it, then it >> ends up VALID ENFORCED. If we take this view of the world (with which, >> I repeat, I disagree) then we must keep track of whether the constraint >> was valid or not valid to start with. And this means that we need to >> keep convalidated=true _regardless_ of whether conenforced is false. >> So in this view of the world there aren't three states but four. >> >> I would prefer there to be three states as well, but apparently I'm >> outvoted on this. > > > Sounds like we agree. I think the problem is with the statement in the linked discussion that “If the constraint is VALID and later marked as NOT ENFORCED, changing it to ENFORCED should also keep it VALID.” This ignores that if it is changed to NOT ENFORCED that should immediately change it to NOT VALID if it is not already so. > > Has anybody argued for how it makes any sense at all to have a constraint that is VALID (and therefore will be assumed to be true by the planner), yet NOT ENFORCED (and therefore may well not be true)? What next, a patch to the planner so that it only treats as true constraints that are both VALID and ENFORCED? I have been asking a different question: What's the use of not-enforced constraints if we don't allow VALID, NOT ENFORCED state for them? OTOH, consider an application which "knows" that the constraint is valid for the data (either because of checks at application level, or because the data was replicated from some other system where the cosntraints were applied). It's a natural ask to use the constraints for, say optimization, but don't take unnecessary overhead of validating them. VALID, NOT ENFORCED state helps in such a scenario. Of course an application can misuse it (just like stable marking on a function), but well ... they will be penalised for their misuse. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-12 11:13 Álvaro Herrera <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Álvaro Herrera @ 2025-02-12 11:13 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Isaac Morland <[email protected]>; Amul Sul <[email protected]>; Peter Eisentraut <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On 2025-Feb-12, Ashutosh Bapat wrote: > I have been asking a different question: What's the use of > not-enforced constraints if we don't allow VALID, NOT ENFORCED state > for them? That's a question for the SQL standards committee. They may serve schema documentation purposes, for example. https://www.postgresql.eu/events/pgconfeu2024/schedule/session/5677-exploring-postgres-databases-wit... > OTOH, consider an application which "knows" that the constraint is > valid for the data (either because of checks at application level, or > because the data was replicated from some other system where the > cosntraints were applied). It's a natural ask to use the constraints > for, say optimization, but don't take unnecessary overhead of > validating them. VALID, NOT ENFORCED state helps in such a scenario. > Of course an application can misuse it (just like stable marking on a > function), but well ... they will be penalised for their misuse. I disagree that we should see a VALID NOT ENFORCED constraint as one that can be used for query optimization purposes. This is only going to bring users pain, because it's far too easy to misuse and they will get wrong query results, possibly without knowing for who knows how long. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "El número de instalaciones de UNIX se ha elevado a 10, y se espera que este número aumente" (UPM, 1972) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: NOT ENFORCED constraint feature @ 2025-02-12 12:41 Peter Eisentraut <[email protected]> parent: Álvaro Herrera <[email protected]> 1 sibling, 0 replies; 7+ messages in thread From: Peter Eisentraut @ 2025-02-12 12:41 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; Isaac Morland <[email protected]>; +Cc: Amul Sul <[email protected]>; Ashutosh Bapat <[email protected]>; jian he <[email protected]>; pgsql-hackers; Joel Jacobson <[email protected]>; Suraj Kharage <[email protected]> On 11.02.25 14:36, Álvaro Herrera wrote: > On 2025-Feb-10, Isaac Morland wrote: > >> I'm having a lot of trouble understanding the operational distinction >> between your 'u' and 'U'. If it's not enforced, it cannot be assumed to be >> valid, regardless of whether it was valid in the past. I'm not sure what I >> think of a single character vs. 2 booleans, but there are only 3 sensible >> states either way: valid enforced, invalid enforced, and invalid unenforced. > > I kinda agree with you and would prefer that things were that way as > well. But look at the discussion starting at > https://postgr.es/m/CAExHW5tV23Sw+Nznv0KpdNg_t7LrXY1WM9atiC=eKKSsKHSnuQ@mail.gmail.com > whereby it was apparently established that if you have a > NOT VALID NOT ENFORCED > constraint, and you make it enforced, then you should somehow end up > with a NOT VALID ENFORCED constraint, which says to me that we need to > store the fact that the constraint was NOT VALID to start with; and > correspondingly if it's VALID NOT ENFORCED and you enforce it, then it > ends up VALID ENFORCED. If we take this view of the world (with which, > I repeat, I disagree) then we must keep track of whether the constraint > was valid or not valid to start with. And this means that we need to > keep convalidated=true _regardless_ of whether conenforced is false. > So in this view of the world there aren't three states but four. > > I would prefer there to be three states as well, but apparently I'm > outvoted on this. Just to make this a bit more confusing, here is another interpretation of the state NOT ENFORCED VALID (they call it DISABLE VALIDATE): https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/constraint.html#GUID-1055EA97-BA... """ DISABLE VALIDATE disables the constraint and drops the index on the constraint, but keeps the constraint valid. This feature is most useful in data warehousing situations, because it lets you load large amounts of data while also saving space by not having an index. This setting lets you load data from a nonpartitioned table into a partitioned table using the exchange_partition_subpart clause of the ALTER TABLE statement or using SQL*Loader. All other modifications to the table (inserts, updates, and deletes) by other SQL statements are disallowed. """ ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-02-12 12:41 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-07-16 20:08 [PATCH v3 4/4] Use hidden visibility for shared libraries where possible. Andres Freund <[email protected]> 2025-02-10 19:30 Re: NOT ENFORCED constraint feature Isaac Morland <[email protected]> 2025-02-11 13:36 ` Re: NOT ENFORCED constraint feature Álvaro Herrera <[email protected]> 2025-02-11 15:39 ` Re: NOT ENFORCED constraint feature Isaac Morland <[email protected]> 2025-02-12 04:47 ` Re: NOT ENFORCED constraint feature Ashutosh Bapat <[email protected]> 2025-02-12 11:13 ` Re: NOT ENFORCED constraint feature Álvaro Herrera <[email protected]> 2025-02-12 12:41 ` Re: NOT ENFORCED constraint feature Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox