Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index
Hi, Shubham! On Wed, Nov 1, 2017 at 12:10 AM, Shubham Barai wrote: > On 9 October 2017 at 18:57, Alexander Korotkov > wrote: > >> Now, ITSM that predicate locks and conflict checks are placed right for >> now. >> However, it would be good to add couple comments to gistdoinsert() whose >> would state why do we call CheckForSerializableConflictIn() in these >> particular places. >> >> I also take a look at isolation tests. You made two separate test specs: >> one to verify that serialization failures do fire, and another to check >> there are no false positives. >> I wonder if we could merge this two test specs into one, but use more >> variety of statements with different keys for both inserts and selects. >> > > Please find the updated version of patch here. I have made suggested > changes. > In general, patch looks good for me now. I just see some cosmetic issues. /* > + *Check for any r-w conflicts (in serialisation isolation level) > + *just before we intend to modify the page > + */ > + CheckForSerializableConflictIn(r, NULL, stack->buffer); > + /* Formatting doesn't look good here. You've missed space after star sign in the comment. You also missed newline after CheckForSerializableConflictIn() call. Also, you've long comment lines in predicate-gist.spec. Please, break long comments into multiple lines. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel
On Thu, Nov 2, 2017 at 3:47 AM, Peter Eisentraut wrote: > So to summarize, I think there is interest in this functionality, but > the patch needs some work. This patch has been waiting for input from its author for three weeks, so I am marking it as returned with feedback. -- Michael
Re: [HACKERS] Is it time to kill support for very old servers?
On Fri, Nov 3, 2017 at 9:30 PM, Michael Paquier wrote: > On Mon, Oct 16, 2017 at 10:08 PM, Andres Freund wrote: >> On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote: >>> On 9/20/17 04:32, Andres Freund wrote: >>> > Here's what I roughly was thinking of. I don't quite like the name, and >>> > the way the version is specified for libpq (basically just the "raw" >>> > integer). >>> >>> "forced_protocol_version" reads wrong to me. I think >>> "force_protocol_version" might be better. Other than that, no issues >>> with this concept. >> >> Yea, I agree. I've read through the patch since, and it struck me as >> odd. Not sure how I came up with it... > > Andres, could you update the patch? Seeing no activity for three weeks and as we are close to the end of the CF, I am marking this one as returned with feedback. -- Michael
Re: [HACKERS] Fix bloom WAL tap test
On Mon, Nov 13, 2017 at 7:13 PM, Alexander Korotkov
wrote:
> On Fri, Nov 10, 2017 at 9:12 PM, Tom Lane wrote:
>> I wrote:
>> > Is there anything we can do to cut the runtime of the TAP test to
>> > the point where running it by default wouldn't be so painful?
>>
>> As an experiment, I tried simply cutting the size of the test table 10X:
>>
>> diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
>> index 1b319c9..566abf9 100644
>> --- a/contrib/bloom/t/001_wal.pl
>> +++ b/contrib/bloom/t/001_wal.pl
>> @@ -57,7 +57,7 @@ $node_standby->start;
>> $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;");
>> $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t
>> text);");
>> $node_master->safe_psql("postgres",
>> -"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,10) i;"
>> +"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,1) i;"
>> );
>> $node_master->safe_psql("postgres",
>> "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 =
>> 3);");
>> @@ -72,7 +72,7 @@ for my $i (1 .. 10)
>> test_index_replay("delete $i");
>> $node_master->safe_psql("postgres", "VACUUM tst;");
>> test_index_replay("vacuum $i");
>> - my ($start, $end) = (11 + ($i - 1) * 1, 10 + $i *
>> 1);
>> + my ($start, $end) = (10001 + ($i - 1) * 1000, 1 + $i * 1000);
>> $node_master->safe_psql("postgres",
>> "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series($start,$end) i;"
>> );
>>
>> This about halved the runtime of the TAP test, and it changed the coverage
>> footprint not at all according to lcov. (Said coverage is only marginally
>> better than what we get without running the bloom TAP test, AFAICT.)
>>
>> It seems like some effort could be put into both shortening this test
>> and improving the amount of code it exercises.
>
> Thank you for committing patch which fixes tap test.
> I'll try to improve coverage of this test and reduce its run time.
I am marking this CF entry as committed, as the switch to psql_safe
has been committed. In order to improve the run time and coverage of
the tests. Let's spawn a new thread.
--
Michael
Re: [HACKERS] Crash on promotion when recovery.conf is renamed
On Mon, Oct 2, 2017 at 3:29 PM, Michael Paquier wrote: > On Mon, Oct 2, 2017 at 8:13 AM, Daniel Gustafsson wrote: >> I’ve moved this to the next CF, but since this no longer applies cleanly I’ve >> reset it to Waiting for author. > > Thanks Daniel for the reminder. Attached are rebased patches. This > thing rots easily... This set of patches still applies, and is marked as ready for committer. Are any of the committers cited on this thread, aka Magnus, Heikki, Tom interested in this patch set? Or not? We are close to the end of CF 2017-11, so I am bumping it to the next one. -- Michael
Re: [HACKERS] More stats about skipped vacuums
At Mon, 27 Nov 2017 10:03:25 +0900, Michael Paquier wrote in > On Mon, Nov 27, 2017 at 5:19 AM, Tom Lane wrote: > > Robert Haas writes: > >> On Sat, Nov 25, 2017 at 12:09 PM, Tom Lane wrote: > >>> Mumble. It's a property I'm pretty hesitant to give up, especially > >>> since the stats views have worked like that since day one. It's > >>> inevitable that weakening that guarantee would break peoples' queries, > >>> probably subtly. > > > >> You mean, queries against the stats views, or queries in general? If > >> the latter, by what mechanism would the breakage happen? > > > > Queries against the stats views, of course. > > There has been much discussion on this thread, and the set of patches > as presented may hurt performance for users with a large number of > tables, so I am marking it as returned with feedback. > -- > Michael Hmmm. Okay, we must make stats collector more effeicient if we want to have additional counters with smaller significance in the table stats. Currently sizeof(PgStat_StatTabEntry) is 168 bytes. The whole of the patchset increases it to 232 bytes. Thus the size of a stat file for a database with 1 tables increases from about 1.7MB to 2.4MB. DSM and shared dynahash is not dynamically expandable so placing stats on shared hash doesn't seem effective. Stats as a regular table could work but it seems too-much. Is it acceptable that adding a new section containing this new counters, which is just loaded as a byte sequence and parsing (and filling the corresponding hash) is postponed until a counter in the section is really requested? The new counters need to be shown in a separate stats view (maybe named pg_stat_vacuum). regards, -- Kyotaro Horiguchi NTT Open Source Software Center
[PATCH] Atomic pgrename on Windows
Hi!
It's assumed in PostgreSQL codebase that pgrename atomically replaces
target file with source file even if target file is open and being read by
another process. And this assumption is true on Linux, but it's false on
Windows. MoveFileEx() triggers an error when target file is open (and
accordingly locked). Some our customers has been faced such errors while
operating heavily loaded PostgreSQL instance on Windows.
LOG could not rename temporary statistics file "pg_stat_tmp/global.tmp" to
"pg_stat_tmp/global.stat": Permission denied
I've managed to reproduce this situation. Reliable reproducing of this
issue required patch to PostgreSQL core. I've written
slow-read-statfiles.patch for artificial slowdown
of pgstat_read_statsfiles() – sleep 100 ms after each statfile entry. If
you run make-100-dbs.sql on patched version, and then few times execute
select pg_stat_get_tuples_inserted('t1'::regclass);
in psql, then you would likely get the error above on Windows.
Attached patch atomic-pgrename-windows-1.patch fixes this problem. It
appears to be possible to atomically replace file on Windows –
ReplaceFile() does that. ReplaceFiles() requires target file to exist,
this is why we still need to call MoveFileEx() when it doesn't exist.
This patch is based on work of Victor Spirin who was asked by Postgres Pro
to research this problem.
--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
slow-read-statfiles.patch
Description: Binary data
make-100-dbs.sql
Description: Binary data
atomic-pgrename-windows-1.patch
Description: Binary data
Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently
A few general comments.
+ FreeSpaceMapVacuum(onerel, 64);
Just want to know why '64' is used here? It's better to give a description.
+else
+ {
+ newslot = fsm_get_avail(page, 0);
+ }
Since there is only one line in the else the bracket will not be needed.
And there in one more space ahead of else which should be removed.
+ if (nindexes == 0 &&
+ (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
vacuum_fsm_every_pages)
+ {
+ /* Vacuum the Free Space Map */
+ FreeSpaceMapVacuum(onerel, 0);
+ vacuumed_pages_at_fsm_vac = vacuumed_pages;
+ }
vacuumed_pages_at_fsm_vac is initialised with value zero and seems no
chance to go into the bracket.
Regards,
Jing Wang
Fujitsu Australia
Re: [HACKERS] GnuTLS support
On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson wrote:
> Hm, after reading more of our MSVC code it seems like building with MSVC
> does not really use switch, people rather have to create a config.pl. Is
> that correct? The MSVC scripts also seems to only support uuid-ossp which it
> just calls $config->{uuid}.
>
> If so we could either have:
>
> $config->{ssl} = "openssl";
> $config->{sslpath} = "/path/to/openssl";
Using this one may actually finish by being cleaner as there is no
need to cross-check for both options defined.
--
Michael
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
(2017/11/27 7:56), Tom Lane wrote: Etsuro Fujita writes: [ fix-rewrite-tlist-v4.patch ] I started reviewing this patch. Great! I did not much like the fact that it effectively moved rewriteTargetListUD to a different file and renamed it. That seems like unnecessary code churn, plus it breaks the analogy with rewriteTargetListIU, plus it will make back-patching harder (since that code isn't exactly the same in back branches). I see little reason why we can't leave it where it is and just make it non-static. It's not like there's no other parts of the rewriter that the planner calls. Agreed. Best regards, Etsuro Fujita
Re: [HACKERS] pg_stat_wal_write statistics view
On Wed, Nov 8, 2017 at 8:46 AM, Robert Haas wrote: > On Tue, Nov 7, 2017 at 4:31 AM, Haribabu Kommi > wrote: > >> Updated patch attached. > > Patch rebased. > > I think the earlier concerns about the performance impact of this are > probably very valid concerns, and I don't see how the new version of > the patch gets us much closer to solving them. > I will check the performance with the changes of removing the stats collector usage and provide the details. > I am also not sure I understand how the backend_write_blocks column is > intended to work. The only call to pgstat_send_walwrites() is in > WalWriterMain, so where do the other backends report anything? > With the current patch, All the backends update the stats in shared memory structure and only WAL writer process gathers the stats and share with the stats collector. > Also, if there's only ever one global set of counters (as opposed to > one per table, say) then why use the stats collector machinery for > this at all, vs. having a structure in shared memory that can be > updated directly? It seems like adding a lot of overhead for no > functional benefit. > Yes, I agree that using stats collector for these stats is an overhead. I change the patch to use just the shared memory structure and gather the performance results and post it to the next commitfest. Currently I marked the patch as "returned with feedback" in the ongoing commitfest. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE
Hi All, This is a patch for current_database working on ALTER ROLE/GRANT/REVOKE statements which should be applied after the previous patch "comment_on_current_database_no_pgdump_v4.4.patch". By using the patch the CURRENT_DATABASE can working in the following SQL statements: ALTER ROLE ... IN DATABASE CURRENT_DATABASE SET/RESET configuration_parameter GRANT ... ON DATABASE CURRENT_DATABASE TO role_specification ... REVOKE ... ON DATABASE CURRENT_DATABASE FROM ... Regards, Jing Wang Fujitsu Australia current_database_on_grant_revoke_role_v4.4.patch Description: Binary data
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
On Wed, Nov 22, 2017 at 11:32 AM, Masahiko Sawada wrote: > On Wed, Nov 22, 2017 at 5:25 AM, Robert Haas wrote: >> On Mon, Nov 20, 2017 at 5:19 PM, Masahiko Sawada >> wrote: >>> Attached updated version patch. I've moved only relation extension >>> locks out of heavy-weight lock as per discussion so far. >>> >>> I've done a write-heavy benchmark on my laptop; loading 24kB data to >>> one table using COPY by 1 client, for 10 seconds. The through-put of >>> patched is 10% better than current HEAD. The result of 5 times is the >>> following. >>> >>> - PATCHED - >>> tps = 178.791515 (excluding connections establishing) >>> tps = 176.522693 (excluding connections establishing) >>> tps = 168.705442 (excluding connections establishing) >>> tps = 158.158009 (excluding connections establishing) >>> tps = 161.145709 (excluding connections establishing) >>> >>> - HEAD - >>> tps = 147.079803 (excluding connections establishing) >>> tps = 149.079540 (excluding connections establishing) >>> tps = 149.082275 (excluding connections establishing) >>> tps = 148.255376 (excluding connections establishing) >>> tps = 145.542552 (excluding connections establishing) >>> >>> Also I've done a micro-benchmark; calling LockRelationForExtension and >>> UnlockRelationForExtension tightly in order to measure the number of >>> lock/unlock cycles per second. The result is, >>> PATCHED = 3.95892e+06 (cycles/sec) >>> HEAD = 1.15284e+06 (cycles/sec) >>> The patched is 3 times faster than current HEAD. >>> >>> Attached updated patch and the function I used for micro-benchmark. >>> Please review it. >> >> That's a nice speed-up. >> >> How about a preliminary patch that asserts that we never take another >> heavyweight lock while holding a relation extension lock? >> > > Agreed. Also, since we disallow to holding more than one locks of > different relations at once I'll add an assertion for it as well. > > I think we no longer need to pass the lock level to > UnloclRelationForExtension(). Now that relation extension lock will be > simple we can release the lock in the mode that we used to acquire > like LWLock. > Attached latest patch incorporated all comments so far. Please review it. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center Moving_extension_lock_out_of_heavyweight_lock_v7.patch Description: Binary data
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On 11/13/2017 12:32 PM, Mark Rofail wrote: == The @>> operator I would argue that allocating an array of datums and building an array would have the same complexity I am not sure what you mean here. Just because something has the same complexity does not mean there can't be major performance differences. I have spend a lot of time working on this operator and would like to benefit from it. How should I go about this ? Start a new patch ? I am still not sure what you mean here. Feel free to add @>> to this patch if you like. You may want to submit it as two patch files (but please keep them as the same commitfest entry) but I leave that decision all up to you. So the two main issues we remain to resolve are MATCH FULL and the RI_Initial_Check() query refactoring. The problem is that I am not one of the original authors and have not touched this part of the code. I understand the problem but it will take some time for me to understand how to resolve everything. Cool, feel free to ask if you need some assistance. I want this patch. Andreas
Re: [HACKERS] GnuTLS support
On 11/27/2017 02:20 AM, Michael Paquier wrote:
On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson wrote:
The script for the windows version takes the
--with-openssl= switch so that cannot just be translated to a single
--with-ssl switch. Should to have both --with-openssl and --with-gnutls or
--with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know
the Windows build code very well (or really at all).
By default --with-openssl does not take a path with ./configure. When
using gnutls, do the name of the libraries and the binaries generated
change compared to openssl? If yes, and I guess that it is the case,
you will need to be able to make the difference between gnutls and
openssl anyway as the set of perl scripts in src/tools/msvc need to
make the difference with deliverables at name-level. I would be
personally fine with just listing gnutls in the list of options and
comment it as --with-ssl=, and change the openssl comment to
match that.
Hm, after reading more of our MSVC code it seems like building with MSVC
does not really use switch, people rather have to create a config.pl. Is
that correct? The MSVC scripts also seems to only support uuid-ossp
which it just calls $config->{uuid}.
If so we could either have:
$config->{ssl} = "openssl";
$config->{sslpath} = "/path/to/openssl";
or
$config->{ssl} = "openssl";
$config->{openssl} = "/path/to/openssl";
or
$config->{openssl} = "/path/to/openssl";
vs
$config->{gnutls} = "/path/to/gnutls";
Andreas
Typo in config_default.pl comment
Hi,
There is a --with-tls in the comments in config_default.pl which should
be --with-tcl.
Andreas
diff --git a/src/tools/msvc/config_default.pl b/src/tools/msvc/config_default.pl
index 4d69dc2a2e..d7a9fc5039 100644
--- a/src/tools/msvc/config_default.pl
+++ b/src/tools/msvc/config_default.pl
@@ -18,7 +18,7 @@ our $config = {
icu => undef,# --with-icu=
nls => undef,# --enable-nls=
tap_tests => undef,# --enable-tap-tests
- tcl => undef,# --with-tls=
+ tcl => undef,# --with-tcl=
perl => undef,# --with-perl
python=> undef,# --with-python=
openssl => undef,# --with-openssl=
Re: [HACKERS] GnuTLS support
On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson wrote: > The script for the windows version takes the > --with-openssl= switch so that cannot just be translated to a single > --with-ssl switch. Should to have both --with-openssl and --with-gnutls or > --with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know > the Windows build code very well (or really at all). By default --with-openssl does not take a path with ./configure. When using gnutls, do the name of the libraries and the binaries generated change compared to openssl? If yes, and I guess that it is the case, you will need to be able to make the difference between gnutls and openssl anyway as the set of perl scripts in src/tools/msvc need to make the difference with deliverables at name-level. I would be personally fine with just listing gnutls in the list of options and comment it as --with-ssl=, and change the openssl comment to match that. -- Michael
Re: [HACKERS] GnuTLS support
On 11/20/2017 02:56 AM, Michael Paquier wrote:
On Mon, Nov 20, 2017 at 9:42 AM, Tomas Vondra
wrote:
If I get it right we ignore gnutls and use openssl (as it's the first
checked in #ifdefs). Shouldn't we enforce in configure that only one TLS
implementation is enabled? Either by some elaborate check, or by
switching to something like
--with-ssl=(openssl|gnutls)
WIth potential patches coming to use macos' SSL implementation or
Windows channel, there should really be only one implementation
available at compile time. That's more simple as a first step as well.
So +1 for the --with-ssl switch.
I have now implemented this in the attached patch (plus added support
for channel binding and rebased it) but I ran into one issue which I
have not yet solved. The script for the windows version takes the
--with-openssl= switch so that cannot just be translated to a
single --with-ssl switch. Should to have both --with-openssl and
--with-gnutls or --with-ssl=(openssl|gnutls) and --with-ssl-path=?
I also do not know the Windows build code very well (or really at all).
Andreas
diff --git a/configure b/configure
index 6c4d743b35..76470af626 100755
--- a/configure
+++ b/configure
@@ -707,7 +707,7 @@ UUID_EXTRA_OBJS
with_uuid
with_systemd
with_selinux
-with_openssl
+with_ssl
krb_srvtab
with_python
with_perl
@@ -834,6 +834,7 @@ with_pam
with_bsd_auth
with_ldap
with_bonjour
+with_ssl
with_openssl
with_selinux
with_systemd
@@ -1528,7 +1529,8 @@ Optional Packages:
--with-bsd-auth build with BSD Authentication support
--with-ldap build with LDAP support
--with-bonjour build with Bonjour support
- --with-openssl build with OpenSSL support
+ --with-ssl=LIB build with TLS support from LIB (openssl,gnutls)
+ --with-openssl obsolete spelling of --with-ssl=openssl
--with-selinux build with SELinux support
--with-systemd build with systemd support
--without-readline do not use GNU Readline nor BSD Libedit for editing
@@ -5961,10 +5963,34 @@ $as_echo "$with_bonjour" >&6; }
#
-# OpenSSL
+# TLS library
#
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with OpenSSL support" >&5
-$as_echo_n "checking whether to build with OpenSSL support... " >&6; }
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking which SSL library to build with" >&5
+$as_echo_n "checking which SSL library to build with... " >&6; }
+
+
+
+# Check whether --with-ssl was given.
+if test "${with_ssl+set}" = set; then :
+ withval=$with_ssl;
+ case $withval in
+yes)
+ as_fn_error $? "argument required for --with-ssl option" "$LINENO" 5
+ ;;
+no)
+ as_fn_error $? "argument required for --with-ssl option" "$LINENO" 5
+ ;;
+*)
+
+ ;;
+ esac
+
+fi
+
+
+if test x"$with_ssl" = x"" ; then
+ with_ssl=no
+fi
@@ -5973,9 +5999,7 @@ if test "${with_openssl+set}" = set; then :
withval=$with_openssl;
case $withval in
yes)
-
-$as_echo "#define USE_OPENSSL 1" >>confdefs.h
-
+ :
;;
no)
:
@@ -5991,8 +6015,23 @@ else
fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_openssl" >&5
-$as_echo "$with_openssl" >&6; }
+if test "$with_ssl" = openssl ; then
+ with_ssl=openssl
+fi
+
+if test "$with_ssl" = openssl ; then
+
+$as_echo "#define USE_OPENSSL 1" >>confdefs.h
+
+elif test "$with_ssl" = gnutls ; then
+
+$as_echo "#define USE_GNUTLS 1" >>confdefs.h
+
+elif test "$with_ssl" != no ; then
+ as_fn_error $? "--with-ssl must specify one of openssl or gnutls" "$LINENO" 5
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_ssl" >&5
+$as_echo "$with_ssl" >&6; }
#
@@ -9911,7 +9950,7 @@ fi
fi
fi
-if test "$with_openssl" = yes ; then
+if test "$with_ssl" = openssl ; then
if test "$PORTNAME" != "win32"; then
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for CRYPTO_new_ex_data in -lcrypto" >&5
$as_echo_n "checking for CRYPTO_new_ex_data in -lcrypto... " >&6; }
@@ -10169,6 +10208,94 @@ done
fi
+if test "$with_ssl" = gnutls ; then
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing gnutls_init" >&5
+$as_echo_n "checking for library containing gnutls_init... " >&6; }
+if ${ac_cv_search_gnutls_init+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+/* Override any GCC internal prototype to avoid an error.
+ Use char because int might match the return type of a GCC
+ builtin and then its argument prototype would still apply. */
+#ifdef __cplusplus
+extern "C"
+#endif
+char gnutls_init ();
+int
+main ()
+{
+return gnutls_init ();
+ ;
+ return 0;
+}
+_ACEOF
+for ac_lib in '' gnutls; do
+ if test -z "$ac_lib"; then
+ac_res="none required"
+ else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib $ac_func_search_save_LIBS"
+ fi
+ if ac_fn_c_try_link "$LINENO"; then :
+ ac_cv_search_gnutls_init=$
Re: [HACKERS] More stats about skipped vacuums
On Mon, Nov 27, 2017 at 5:19 AM, Tom Lane wrote: > Robert Haas writes: >> On Sat, Nov 25, 2017 at 12:09 PM, Tom Lane wrote: >>> Mumble. It's a property I'm pretty hesitant to give up, especially >>> since the stats views have worked like that since day one. It's >>> inevitable that weakening that guarantee would break peoples' queries, >>> probably subtly. > >> You mean, queries against the stats views, or queries in general? If >> the latter, by what mechanism would the breakage happen? > > Queries against the stats views, of course. There has been much discussion on this thread, and the set of patches as presented may hurt performance for users with a large number of tables, so I am marking it as returned with feedback. -- Michael
Re: [HACKERS] More stats about skipped vacuums
On Mon, Nov 27, 2017 at 2:49 AM, Tom Lane wrote: > Michael Paquier writes: >> ... Would you think >> that it is acceptable to add the number of index scans that happened >> with the verbose output then? > > I don't have an objection to it, but can't you tell that from VACUUM > VERBOSE already? There should be a "INFO: scanned index" line for > each scan. Of course, sorry for the noise. -- Michael
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
Etsuro Fujita writes: > [ fix-rewrite-tlist-v4.patch ] I started reviewing this patch. I did not much like the fact that it effectively moved rewriteTargetListUD to a different file and renamed it. That seems like unnecessary code churn, plus it breaks the analogy with rewriteTargetListIU, plus it will make back-patching harder (since that code isn't exactly the same in back branches). I see little reason why we can't leave it where it is and just make it non-static. It's not like there's no other parts of the rewriter that the planner calls. I revised the patch along that line, and while at it, refactored preptlist.c a bit to eliminate repeated heap_opens of the target relation. I've not really reviewed any other aspects of the patch yet, but in the meantime, does anyone object to proceeding this way? regards, tom lane diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 4339bbf..f90a6cf 100644 *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** update bar set f2 = f2 + 100 returning * *** 7022,7027 --- 7022,7086 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + delete from bar where f2 < 400; + QUERY PLAN + - + Delete on public.bar +Delete on public.bar +Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.ctid + Filter: (bar.f2 < 400) +-> Foreign Scan on public.bar2 + Output: bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE + (10 rows) + + delete from bar where f2 < 400; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; NOTICE: drop cascades to foreign table foo2 drop table bar cascade; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index ddfec79..8e1bd89 100644 *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** explain (verbose, costs off) *** 1656,1661 --- 1656,1681 update bar set f2 = f2 + 100 returning *; update bar set f2 = f2 + 100 returning *; + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + explain (verbose, costs off) + update bar set f2 = f2 + 100; + update bar set f2 = f2 + 100; + + explain (verbose, costs off) + delete from bar where f2 < 400; + delete from bar where f2 < 400; + + -- cleanup + DRO
Re: Memory error in src/backend/replication/logical/origin.c
> On Nov 26, 2017, at 10:28 AM, Tom Lane wrote: > > Mark Dilger writes: >>boolnulls[Natts_pg_replication_origin]; >>memset(&nulls, 0, sizeof(nulls)); > >> around lines 277 through 303. Patch below. > > AFAIK this is not a bug, though I agree that dropping the "&" is probably > better style. The reason is that applying "&" to an undecorated array > name is basically a no-op, because without "&" the array name would decay > to a pointer anyway. With "&", the address-taking is explicit, but you > still get a pointer to the array, not a pointer to some pointer to the > array. Ain't C fun? Thanks for the refresher on C madness. mark
Re: [HACKERS] More stats about skipped vacuums
Robert Haas writes: > On Sat, Nov 25, 2017 at 12:09 PM, Tom Lane wrote: >> Mumble. It's a property I'm pretty hesitant to give up, especially >> since the stats views have worked like that since day one. It's >> inevitable that weakening that guarantee would break peoples' queries, >> probably subtly. > You mean, queries against the stats views, or queries in general? If > the latter, by what mechanism would the breakage happen? Queries against the stats views, of course. regards, tom lane
Re: [HACKERS] More stats about skipped vacuums
On Sat, Nov 25, 2017 at 12:09 PM, Tom Lane wrote: > Robert Haas writes: >> Of course, the other obvious question is whether we really need a >> consistent snapshot, because that's bound to be pretty expensive even >> if you eliminate the I/O cost. Taking a consistent snapshot across >> all 100,000 tables in the database even if we're only ever going to >> access 5 of those tables doesn't seem like a good or scalable design. > > Mumble. It's a property I'm pretty hesitant to give up, especially > since the stats views have worked like that since day one. It's > inevitable that weakening that guarantee would break peoples' queries, > probably subtly. You mean, queries against the stats views, or queries in general? If the latter, by what mechanism would the breakage happen? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Memory error in src/backend/replication/logical/origin.c
Mark Dilger writes: > boolnulls[Natts_pg_replication_origin]; > memset(&nulls, 0, sizeof(nulls)); > around lines 277 through 303. Patch below. AFAIK this is not a bug, though I agree that dropping the "&" is probably better style. The reason is that applying "&" to an undecorated array name is basically a no-op, because without "&" the array name would decay to a pointer anyway. With "&", the address-taking is explicit, but you still get a pointer to the array, not a pointer to some pointer to the array. Ain't C fun? regards, tom lane
Memory error in src/backend/replication/logical/origin.c
Hackers, boolnulls[Natts_pg_replication_origin]; ... memset(&nulls, 0, sizeof(nulls)); around lines 277 through 303. Patch below. mark diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c index 55382b4b24..88188bd190 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -300,7 +300,7 @@ replorigin_create(char *roname) * Ok, found an unused roident, insert the new row and do a CCI, * so our callers can look it up if they want to. */ - memset(&nulls, 0, sizeof(nulls)); + memset(nulls, 0, sizeof(nulls)); values[Anum_pg_replication_origin_roident - 1] = ObjectIdGetDatum(roident); values[Anum_pg_replication_origin_roname - 1] = roname_d;
Re: has_sequence_privilege() never got the memo
On 11/23/2017 07:16 AM, Peter Eisentraut wrote: > On 11/22/17 22:58, Tom Lane wrote: >> Joe Conway writes: >>> I just noticed that has_sequence_privilege() never got the memo about >>> "WITH GRANT OPTION". Any objections to the attached going back to all >>> supported versions? >> >> That looks odd. Patch certainly makes this case consistent with the >> rest of acl.c, but maybe there's some deeper reason? Peter? > > No I think it was just forgotten. Pushed. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] More stats about skipped vacuums
Michael Paquier writes: > ... Would you think > that it is acceptable to add the number of index scans that happened > with the verbose output then? I don't have an objection to it, but can't you tell that from VACUUM VERBOSE already? There should be a "INFO: scanned index" line for each scan. regards, tom lane
Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA
2017-11-06 18:07 GMT+03:00 Sokolov Yura : > > On 2017-10-20 11:54, Sokolov Yura wrote: >> >> Hello, >> >> On 2017-10-19 19:46, Andres Freund wrote: >>> >>> On 2017-10-19 14:36:56 +0300, Sokolov Yura wrote: > > + init_local_spin_delay(&delayStatus); > > The way you moved this around has the disadvantage that we now do this - > a number of writes - even in the very common case where the lwlock can > be acquired directly. Excuse me, I don't understand fine. Do you complain against init_local_spin_delay placed here? >>> >>> >>> Yes. >> >> >> I could place it before perform_spin_delay under `if (!spin_inited)` if you >> think it is absolutely must. > > > I looked at assembly, and remembered, that last commit simplifies > `init_local_spin_delay` to just two-three writes of zeroes (looks > like compiler combines 2*4byte write into 1*8 write). Compared to > code around (especially in LWLockAcquire itself), this overhead > is negligible. > > Though, I found that there is benefit in calling LWLockAttemptLockOnce > before entering loop with calls to LWLockAttemptLockOrQueue in the > LWLockAcquire (in there is not much contention). And this way, `inline` > decorator for LWLockAttemptLockOrQueue could be omitted. Given, clang > doesn't want to inline this function, it could be the best way. In attach version with LWLockAcquireOnce called before entering loop in LWLockAcquire. With regards, Sokolov Yura lwlock_v5.patch.gz Description: GNU Zip compressed data
Re: [PATCH] Fix crash in int8_avg_combine().
Hi Hadi, On 2017-11-25 22:43:49 -0500, Hadi Moshayedi wrote: > While doing some tests on REL_10_STABLE, I was getting run-time exceptions > at int8_avg_combine() at the following line: > > state1->sumX = state2->sumX; > > After some debugging, I noticed that palloc()’s alignment is 8-bytes, while > this statement (which moves a __int128 from one memory location to another > memory location) expects 16-byte memory alignments. So when either state1 > or state2 is not 16-byte aligned, this crashes. > > When I disassemble the code, the above statement is translated to a pair of > movdqa and movaps assignments when compiled with -O2: > > movdqa c(%rdx), %xmm0 > movaps %xmm0, c(%rcx) > > Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual, > Volume 2”, both of these instructions expect 16-byte aligned memory > locations, or a general-protection exception will be generated. Nicely analyzed. [Un]fortunately the bug has already been found and fixed: https://git.postgresql.org/pg/commitdiff/619a8c47da7279c186bb57cc16b26ad011366b73 Will be included in the next set of back branch releases. > diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h > index 869c59dc85..2dc59e89cd 100644 > --- a/src/include/utils/memutils.h > +++ b/src/include/utils/memutils.h > @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContext > parent, > * Few callers should be interested in this, but tuplesort/tuplestore need > * to know it. > */ > -#define ALLOCSET_SEPARATE_THRESHOLD 8192 > +#define ALLOCSET_SEPARATE_THRESHOLD 16384 Huh, what's that about in this context? Greetings, Andres Freund
Re: [HACKERS] Cache lookup errors with functions manipulation object addresses
On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier wrote: > Attached is a rebased patch set. Álvaro, as you have introduced most > of the problems with 4464303 & friends dated of 2015 when you > introduced get_object_address(), could you look at this patch set? Moved to next commit fest. -- Michael
Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256
On Tue, Nov 21, 2017 at 1:36 PM, Michael Paquier
wrote:
> So attached are rebased patches:
> - 0001 to introduce the connection parameter saslchannelbinding, which
> allows libpq to enforce the type of channel binding used during an
> exchange.
> - 0002 to add tls-endpoint as channel binding type, which is where 0001
> shines.
Attached is a rebased patch set, documentation failing to compile. I
am moving at the same time this patch set to the next commit fest.
--
Michael
From bdd25121ba7c1916d280f97c8e1a280ad26ea60c Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 10 Oct 2017 22:04:22 +0900
Subject: [PATCH 2/2] Implement channel binding tls-server-end-point for SCRAM
As referenced in RFC 5929, this channel binding is not the default value
and uses a hash of the certificate as binding data. On the frontend, this
can be resumed in getting the data from SSL_get_peer_certificate() and
on the backend SSL_get_certificate().
The hashing algorithm needs also to switch to SHA-256 if the signature
algorithm is MD5 or SHA-1, so let's be careful about that.
---
doc/src/sgml/protocol.sgml | 5 +-
src/backend/libpq/auth-scram.c | 26 ---
src/backend/libpq/auth.c | 8 +++-
src/backend/libpq/be-secure-openssl.c| 61 +
src/include/libpq/libpq-be.h | 1 +
src/include/libpq/scram.h| 4 +-
src/interfaces/libpq/fe-auth-scram.c | 24 +++---
src/interfaces/libpq/fe-auth.c | 12 -
src/interfaces/libpq/fe-auth.h | 4 +-
src/interfaces/libpq/fe-secure-openssl.c | 78
src/interfaces/libpq/libpq-int.h | 1 +
src/test/ssl/t/002_scram.pl | 5 +-
12 files changed, 210 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 8174e3defa..365f72b51d 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1576,8 +1576,9 @@ the password is in.
Channel binding is supported in PostgreSQL builds with
SSL support. The SASL mechanism name for SCRAM with channel binding
-is SCRAM-SHA-256-PLUS. The only channel binding type
-supported at the moment is tls-unique, defined in RFC 5929.
+is SCRAM-SHA-256-PLUS. Two channel binding types are
+supported at the moment: tls-unique, which is the default,
+and tls-server-end-point, both defined in RFC 5929.
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 22103ce479..8f96e3927e 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -113,6 +113,8 @@ typedef struct
bool ssl_in_use;
const char *tls_finished_message;
size_t tls_finished_len;
+ const char *certificate_hash;
+ size_t certificate_hash_len;
char *channel_binding_type;
int iterations;
@@ -175,7 +177,9 @@ pg_be_scram_init(const char *username,
const char *shadow_pass,
bool ssl_in_use,
const char *tls_finished_message,
- size_t tls_finished_len)
+ size_t tls_finished_len,
+ const char *certificate_hash,
+ size_t certificate_hash_len)
{
scram_state *state;
bool got_verifier;
@@ -186,6 +190,8 @@ pg_be_scram_init(const char *username,
state->ssl_in_use = ssl_in_use;
state->tls_finished_message = tls_finished_message;
state->tls_finished_len = tls_finished_len;
+ state->certificate_hash = certificate_hash;
+ state->certificate_hash_len = certificate_hash_len;
state->channel_binding_type = NULL;
/*
@@ -852,13 +858,15 @@ read_client_first_message(scram_state *state, char *input)
}
/*
- * Read value provided by client; only tls-unique is supported
- * for now. (It is not safe to print the name of an
- * unsupported binding type in the error message. Pranksters
- * could print arbitrary strings into the log that way.)
+ * Read value provided by client; only tls-unique and
+ * tls-server-end-point are supported for now. (It is
+ * not safe to print the name of an unsupported binding
+ * type in the error message. Pranksters could print
+ * arbitrary strings into the log that way.)
*/
channel_binding_type = read_attr_value(&input, 'p');
-if (strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0)
+if (strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0 &&
+ strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_ENDPOINT) != 0)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
(errmsg("unsupported SCRAM channel-binding type";
@@ -1116,6 +1124,12 @@ read_client_final_message(scram_state *state, char *input)
cbind_data = state->tls_finished_message;
cbind_data_len = state->tls_finished_len;
}
+ else if (strcmp(state->channel_binding_type,
+ SCRAM_CHANNEL_BINDING_TLS_ENDPOINT) == 0)
+ {
+ cbind_data = state->certificate_hash;
+ cbind_data_len = state->certificate_hash_len;
+ }
else
Re: [HACKERS] More stats about skipped vacuums
On Sun, Nov 26, 2017 at 9:59 AM, Tom Lane wrote: > I'd say so ... that's something the average user will never bother with, > and even if they knew to bother, it's far from obvious what to do with > the information. Besides, I don't think you could just save the number > of scans and nothing else. For it to be meaningful, you'd at least have > to know the prevailing work_mem setting and the number of dead tuples > removed ... and then you'd need some info about your historical average > and maximum number of dead tuples removed, so that you knew whether the > last vacuum operation was at all representative. So this is sounding > like quite a lot of new counters, in support of perhaps 0.1% of the > user population. Most people are just going to set maintenance_work_mem > as high as they can tolerate and call it good. In all the PostgreSQL deployments I deal with, the database is embedded with other things running in parallel and memory is something that's shared between components, so being able to tune more precisely any of the *_work_mem parameters has value (a couple of applications are also doing autovacuum tuning at relation-level). Would you think that it is acceptable to add the number of index scans that happened with the verbose output then? Personally I could live with that information. I recall as well a thread about complains that VACUUM VERBOSE is showing already too much information, I cannot put my finger on it specifically now though. With autovacuum_log_min_duration, it is easy enough to trace a vacuum pattern. The thing is that for now the tuning is not that evident, and CPU cycles can be worth saving in some deployments while memory could be extended more easily. -- Michael
Re: [HACKERS] [POC] Faster processing at Gather node
On Sat, Nov 25, 2017 at 9:13 PM, Robert Haas wrote: > On Wed, Nov 22, 2017 at 8:36 AM, Amit Kapila wrote: >>> remove-memory-leak-protection-v1.patch removes the memory leak >>> protection that Tom installed upon discovering that the original >>> version of tqueue.c leaked memory like crazy. I think that it >>> shouldn't do that any more, courtesy of >>> 6b65a7fe62e129d5c2b85cd74d6a91d8f7564608. Assuming that's correct, we >>> can avoid a whole lot of tuple copying in Gather Merge and a much more >>> modest amount of overhead in Gather. Since my test case exercised >>> Gather Merge, this bought ~400 ms or so. >> >> I think Tom didn't installed memory protection in nodeGatherMerge.c >> related to an additional copy of tuple. I could see it is present in >> the original commit of Gather Merge >> (355d3993c53ed62c5b53d020648e4fbcfbf5f155). Tom just avoided applying >> heap_copytuple to a null tuple in his commit >> 04e9678614ec64ad9043174ac99a25b1dc45233a. I am not sure whether you >> are just referring to the protection Tom added in nodeGather.c, If >> so, it is not clear from your mail. > > Yes, that's what I mean. What got done for Gather Merge was motivated > by what Tom did for Gather. Sorry for not expressing the idea more > precisely. > >> I think we don't need the additional copy of tuple in >> nodeGatherMerge.c and your patch seem to be doing the right thing. >> However, after your changes, it looks slightly odd that >> gather_merge_clear_tuples() is explicitly calling heap_freetuple for >> the tuples allocated by tqueue.c, maybe we can add some comment. It >> was much clear before this patch as nodeGatherMerge.c was explicitly >> copying the tuples that it is freeing. > > OK, I can add a comment about that. > Sure, I think apart from that the patch looks good to me. I think a good test of this patch could be to try to pass many tuples via gather merge and see if there is any leak in memory. Do you have any other ideas? >> Isn't it better to explicitly call gather_merge_clear_tuples in >> ExecEndGatherMerge so that we can free the memory for tuples allocated >> in this node rather than relying on reset/free of MemoryContext in >> which those tuples are allocated? > > Generally relying on reset/free of a MemoryContext is cheaper. > Typically you only want to free manually if the freeing would > otherwise not happen soon enough. > Yeah and I think something like that can happen after your patch because now the memory for tuples returned via TupleQueueReaderNext will be allocated in ExecutorState and that can last for long. I think it is better to free memory, but we can leave it as well if you don't feel it important. In any case, I have written a patch, see if you think it makes sense. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com release_memory_at_gather_merge_shutdown_v1.patch Description: Binary data
