Re: [HACKERS] Parallel Seq Scan
On Wed, Apr 1, 2015 at 6:11 PM, Robert Haas wrote: > > On Wed, Apr 1, 2015 at 7:30 AM, Amit Kapila wrote: > >> Also, the new code to propagate > >> XactLastRecEnd won't work right, either. > > > > As we are generating FATAL error on termination of worker > > (bgworker_die()), so won't it be handled in AbortTransaction path > > by below code in parallel-mode patch? > > That will asynchronously flush the WAL, but if the master goes on to > commit, we've wait synchronously for WAL flush, and possibly sync rep. > Okay, so you mean if master backend later commits, then there is a chance of loss of WAL data written by worker. Can't we report the location to master as the patch does in case of Commit in worker? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] PL/pgSQL, RAISE and error context
2015-01-26 16:46 GMT+01:00 Pavel Stehule : > > > 2015-01-26 16:14 GMT+01:00 Tom Lane : > >> Pavel Stehule writes: >> > 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja : >> > I am thinking, so solution >> >> > /* if we are doing RAISE, don't report its location */ >> > if (estate->err_text == raise_skip_msg) >> > return; >> >> > is too simple, and this part should be fixed. This change can be done >> by on >> > plpgsql or libpq side. This is bug, and it should be fixed. >> >> Doing this in libpq is utterly insane. It has not got sufficient context >> to do anything intelligent. The fact that it's not intelligent is exposed >> by the regression test changes that the proposed patch causes, most of >> which do not look like improvements. >> >> > I don't understand. There can be a overhead due useless transformation > some data to client side. But all what it need - errcontext and errlevel is > possible. > > >> Another problem is that past requests to change this behavior have >> generally been to the effect that people wanted *more* context suppressed >> not less, ie they didn't want any CONTEXT lines at all on certain >> messages. So the proposed patch seems to me to be going in exactly the >> wrong direction. >> >> The design I thought had been agreed on was to add some new option to >> plpgsql's RAISE command which would cause suppression of all CONTEXT lines >> not just the most closely nested one. You could argue about whether the >> behavior needs to be 100% backwards compatible or not --- if so, perhaps >> it could be a three-way option all, none, or one line, defaulting to the >> last for backwards compatibility. >> > > I see a problem what should be default behave. When I raise NOTICE, then > I don't need (don't would) to see CONTEXT lines, When I raise EXCEPTION, > then I usually would to see CONTEXT lines. > > Cannot be solution? > I would to wakeup this thread. > > estate->err_text = stmt->elog_level == ERROR ? estate->err_text : > raise_skip_msg ; > Can we do this simple change? It will produce a stackinfo for exceptions and it will not to make mad developers by lot of useless content. Regards Pavel > > Regards > > Pavel > > > > >> >> regards, tom lane >> > >
Re: [HACKERS] Logical decoding (contrib/test_decoding) walsender broken in 9.5 master?
I started bisecting from e6df2e1 (stamp 9.4beta1, good) to 095d401 (bad).
The problem revision appears to be 9402869:
commit 9402869
Author: Heikki Linnakangas
Date: Sat Jan 17 01:14:32 2015 +0200
Advance backend's advertised xmin more aggressively.
which is somewhat logical given the crash location and symptoms.
Bisection procedure used:
For each step:
./configure --enable-debug --enable-cassert --prefix=/home/craig/pg/95
CFLAGS="-Og -ggdb -fno-omit-frame-pointer" && make clean install && make -C
contrib/test_decoding/ clean install && rm -rf ~/tmp/slottest95 &&
PATH=$HOME/pg/95/bin:$PATH initdb -D ~/tmp/slottest95 && cp
~/tmp/{postgresql.conf,pg_hba.conf} ~/tmp/slottest95 &&
PATH=$HOME/pg/95/bin:$PATH PGPORT=5123 postgres -D ~/tmp/slottest95/
then manually:
psql -p 5123 -c 'SELECT pg_create_logical_replication_slot('test',
'test_decoding');'
PGPORT=5123 PATH=$HOME/pg/95/bin:$PATH pg_recvlogical -d postgres -S test
--start -f -
psql -p 5123 -c 'CREATE TABLE x AS SELECT xx FROM generate_series(1,1)
xx;'
I have to go out now; I'll follow up further but wanted to update promptly.
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
auto-updatable view work just for postgresql-9.3 and above (for other version
you still need to define DELETE/UPDATE trigger).
what i see is we just trying to have a work around either with BEFORE/AFTER
trigger or with auto-updatable view
in stright forwad/normale way is just to define INSTEAD OF trigger on the
master table that return NEW so it doesn't break RETURNING, and the actual
tuples returned by the trigger wouldn't actually be inserted in the master
table. after all, that what INSTEAD OF suppose to do.
tom lane : in partitioned table. normally (always), the data is stored in child
tables (i know this not the case for inheritence) . any data inserted in master
table is just an exception/error/design bug or this is just my case. what i
mean is, if some one define master table as empty table (even without having
INSTEAD OF trigger) is not wart (postgresql need to be more flexible, and let
user define thier database architecture the way they like).
also, it would be nice that the example :
INSERT INTO cities (name, population, altitude, state)
VALUES ('New York', NULL, NULL, 'NY');
in the inheritence doc to work, (if we maked passes syntax error checking and
planning phase) next step is to chose between rule and trigger (we already
have instead of rule. we just need instead of trigger ) maybe this not a user
defined one but implicitly.
-Original Message-
From: Dean Rasheed
To: Andres Freund
Cc: Tom Lane ; Robert Haas ; Aliouii
Ali ; pgsql-hackers
Sent: Wed, Apr 1, 2015 8:01 pm
Subject: Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On 1 April 2015 at 18:37, Andres Freund wrote:
> On
2015-04-01 13:29:33 -0400, Tom Lane wrote:
>> As for partitioning, you could do
this:
>>
>> create table parent(...);
>> create table child(...)
inherits(parent); -- repeat as needed
>> create view v as select * from
parent;
>> attach INSTEAD OF triggers to v
>>
>> Now the application deals
only with v, and thinks that's the real
>> table.
>
> Sure, but that's just
making things unnecessarily hard. That then
> requires also defining
UPDATE/DELETE INSTEAD triggers which otherwise
> would just work.
>
No,
because as defined above the view v would be auto-updatable, so
updates and
deletes on v would just do the matching update/delete
on
parent.
Regards,
Dean
Re: [HACKERS] libpq's multi-threaded SSL callback handling is busted
Peter Eisentraut writes: > On 2/12/15 7:28 AM, Jan Urbański wrote: >> For the sake of discussion, here's a patch to prevent stomping on >> previously-set callbacks, racy as it looks. >> >> FWIW, it does fix the Python deadlock and doesn't cause the PHP segfault... > > I don't think this patch would actually fix the problem that was > described after the original bug report > (http://www.postgresql.org/message-id/5436991b.5020...@vmware.com), > namely that another thread acquires a lock while the libpq callbacks are > set and then cannot release the lock if libpq has been shut down in the > meantime. I did test both the Python and the PHP repro scripts and the patch fixed both the deadlock and the segfault. What happens is that Python (for instance) stops over the callback unconditionally. So when libpq gets unloaded, it sees that the currently set callback is no the one it originally set and refrains from NULLing it. There's a small race window there, to be sure, but it's a lot better than what we have now. Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical decoding (contrib/test_decoding) walsender broken in 9.5 master?
On Thu, Apr 2, 2015 at 5:35 PM, Craig Ringer wrote: > I started bisecting from e6df2e1 (stamp 9.4beta1, good) to 095d401 (bad). > The problem revision appears to be 9402869: > > commit 9402869 > Author: Heikki Linnakangas > Date: Sat Jan 17 01:14:32 2015 +0200 > > Advance backend's advertised xmin more aggressively. This rings a bell, I reported a similar issue some weeks back: http://www.postgresql.org/message-id/CAB7nPqQSdx7coHk0D6G=mkjntgyjxpdw+pwiskkssaezfts...@mail.gmail.com And there is a patch. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Supporting TAP tests with MSVC and Windows
Hi all,
(Adding Peter and Heikki in CC for awareness)
Please find attached a WIP patch to support TAP tests with MSVC. The
tests can be kicked with this command:
vcregress tapcheck
There are a couple of things to note with this patch, and I would like
to have some input for some of those things:
1) I have tweaked some code paths to configure a node correctly, with
for example listen_addresses = 'localhost', or disabling local
settings in pg_hba.conf. Does that sound fine?
2) tapcheck does not check if IPC::Run is installed or not. Should we
add a check in the code for that? If yes, should we bypass the test or
fail?
3) Temporary installation needs to be done in the top directory,
actually out of src/, because Install.pm scans the contents of src/ to
fetch for example the .samples files and this leads to bad
interactions. Using this location has as well the advantage to install
the binaries for all the tests only once.
4) pg_rewind tests are disabled for now, I am waiting for the outcome
of 5519f169.8030...@gmx.net
5) ssl tests are disabled for now (haven't looked at that yet). They
should be tested if possible. Still need some investigation.
6) the command to access pg_regress is passed using an environment
variable TESTREGRESS.
7) TAP tests use sometimes top_builddir directly. I think that's ugly,
and if there are no objections I think that we should change it to use
a dedicated environment variable like TESTTOP or similar.
8) Some commands have needed some tweaks because option parsing on
Windows is... Well... sometimes broken. For example those things do
not work on Windows:
createdb foobar3 -E win1252
initdb PGDATA -X XLOGDIR
Note that modifying those commands does not change the test coverage.
9) @Peter: IPC::Run is not reliable when using h->results, by
switching to (h->full_results)[0] I was able to get results reliably
from a child process.
10) This patch needs to have IPC::Run installed. You need to install
it from CPAN, that's the same deal as for OSX.
11) Windows does not support symlink, so some tests of pg_basebackup
cannot be executed. I can live with this restriction.
I think that's all for now. I'll add this patch to the 2015-06 commit fest.
Regards,
--
Michael
diff --git a/.gitignore b/.gitignore
index 8d3af50..3cd37fe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,3 +36,6 @@ lib*.pc
/pgsql.sln.cache
/Debug/
/Release/
+
+# Generated by tests
+/tmp_check/
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 9b77648..d3d8f5f 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -438,6 +438,7 @@ $ENV{CONFIG}="Debug";
vcregress contribcheck
vcregress ecpgcheck
vcregress isolationcheck
+vcregress tapcheck
vcregress upgradecheck
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 149b3d1..091ec0e 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -1,10 +1,26 @@
use strict;
use warnings;
+use Config;
use TestLib;
use Test::More tests => 19;
my $tempdir = TestLib::tempdir;
+sub cleanup_tempdir
+{
+ my $tempdir = shift;
+
+ if ($Config{osname} eq "MSWin32")
+ {
+ system_or_bail "rd /s /q $tempdir";
+ mkdir $tempdir;
+ }
+ else
+ {
+ system_or_bail "rm -rf '$tempdir'/*";
+ }
+}
+
program_help_ok('initdb');
program_version_ok('initdb');
program_options_handling_ok('initdb');
@@ -18,27 +34,26 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ],
mkdir "$tempdir/data4" or BAIL_OUT($!);
command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
-system_or_bail "rm -rf '$tempdir'/*";
-
-command_ok([ 'initdb', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
- 'separate xlog directory');
+cleanup_tempdir $tempdir;
-system_or_bail "rm -rf '$tempdir'/*";
+command_ok([ 'initdb', '-D', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
+ 'separate xlog directory');
+cleanup_tempdir $tempdir;
command_fails(
[ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
'relative xlog directory not allowed');
-system_or_bail "rm -rf '$tempdir'/*";
+cleanup_tempdir $tempdir;
mkdir "$tempdir/pgxlog";
-command_ok([ 'initdb', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
+command_ok([ 'initdb', '-D', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
'existing empty xlog directory');
-system_or_bail "rm -rf '$tempdir'/*";
+cleanup_tempdir $tempdir;
mkdir "$tempdir/pgxlog";
mkdir "$tempdir/pgxlog/lost+found";
-command_fails([ 'initdb', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
+command_fails([ 'initdb', '-D', "$tempdir/data", '-X', "$tempdir/pgxlog" ],
'existing nonempty xlog directory');
-system_or_bail "rm -rf '$tempdir'/*";
-command_ok([ 'initdb', "$tempdir/data", '-T', 'german' ],
+cleanup_tempdir $tempdir;
+command_ok([ 'initdb', '-D', "$tempdir/data", '-T', 'german' ],
'select default dictionary');
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 7e9a776..130f41a 100644
-
Re: [HACKERS] What exactly is our CRC algorithm?
At 2015-03-25 19:18:51 +0200, hlinn...@iki.fi wrote:
>
> I think we'll need a version check there. […]
>
> You want to write that or should I?
I'm not familiar with MSVC at all, so it would be nice if you did it.
> How do you like this latest version of the patch otherwise?
I'm sorry, but I'm still not especially fond of it.
Apart from removing the startup check so that client programs can also
use the best available CRC32C implementation without jumping through
hoops, I don't feel that the other changes buy us very much.
Also, assuming that the point is that people who don't care about CRCs
deeply should nevertheless be able to produce special-purpose binaries
with only the optimal implementation included, we should probably have
some instructions about how to do that.
Thinking about what you were trying to do, I would find an arrangement
roughly like the following to be clearer to follow in terms of adding
new implementations and so on:
#if defined(USE_CRC_SSE42) || …can build SSE4.2 CRC code…
#define HAVE_CRC_SSE42 1
pg_crc32c_sse42() { … }
bool sse42_crc32c_available() { … }
pg_comp_crc32c = pg_crc32c_sse42;
#elif defined(USE_CRC_ARM) || …can build ARM CRC code…
#define HAVE_CRC_ARM 1
pg_crc32c_arm() { … }
bool arm_crc32c_available() { … }
pg_comp_crc32c = pg_crc32c_arm;
#endif
#define CRC_SELECTION 1
#if defined(USE_CRC_SSE42) || defined(USE_CRC_ARM) || …
#undef CRC_SELECTION
#endif
#ifdef CRC_SELECTION
pg_crc32c_sb8() { … }
pg_comp_crc32c_choose(…)
{
pg_comp_crc32c = pg_crc32c_sb8;
#ifdef HAVE_CRC_SSE42
if (sse42_crc32c_available())
pg_comp_crc32c = pg_crc32c_sse42;
#elif …
…
#endif
return pg_comp_crc32c(…);
}
pg_comp_crc32c = pg_crc32c_choose;
#endif
Then someone who wants to force the building of (only) the SSE4.2
implementation can build with -DUSE_CRC_SSE42. And if you turn on
USE_CRC_ARM when you can't build ARM code, it won't build. (The
HAVE_CRC_xxx #defines could also move to configure tests.)
If you don't specify any USE_CRC_xxx explicitly, then it'll build
whichever (probably) one it can, and select it at runtime if it's
available.
All that said, I do recognise that there are all relatively cosmetic
concerns, and I don't object seriously to any of it. On the contrary,
thanks for taking the time to review and work on the patch. Nobody
else has expressed an opinion, so I'll leave it to you to decide whether
to commit as-is, or if you want me to pursue the above approach instead.
In the realm of very minor nitpicking, here are a couple of points I
noticed in crc_instructions.h:
1. I think «if (pg_crc32_instructions_runtime_check())» would read
better if the function were named crc32c_instructions_available or
pg_crc32c_is_hw_accelerated, or something like that.
2. It documents pg_accumulate_crc32c_byte and
pg_accumulate_crc32c_uint64, but actually defines pg_asm_crc32b and
pg_asm_crc32q. If you update the code rather than the documentation,
_update_ may be slightly preferable to _accumulate_, and maybe the
suffixes should be _uint8 and _uint64.
3. The descriptions (e.g. "Add one byte to the current crc value.")
should also probably read "Update the current crc value for the given
byte/eight bytes of data".
Thanks.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GSoC 2015. Support for microvacuum for GiST. Feedback for my proposal.
Hi, all! I have sent GSoC proposal (http://postgresql.nabble.com/GSoC-2015-proposal-Support-for-microvacuum-for-GiST-td5843638.html) about week ago and I haven't received any feedback. So I'm a bit confused. Is the idea of proposal not actual for community or maybe too small for GSoC project? I do want participate in GSoC, but without comments I can't understand what's wrong and improve the proposal. Sincerely, Ilya Ivanitskiy.
Re: [HACKERS] GSoC 2015 proposal: Support for microvacuum for GiST
On 03/26/2015 11:37 PM, Ilia Ivanicki wrote: *Abstract:* Currently support for microvacuum is implemented only for BTree index. But GiST index is so useful and widely used for user defined datatypes instead of btree. During index search it reads page by page. Every tuple on the page in buffer marked as "dead" if it doesn't visible for all transactions. Whenever before receiving next page we check "dead" items and mark current page as "has garbage"[1]. When the page gets full, all the killed items are removed by calling microvacuum[2]. Seems reasonable. Should be a pretty straightforward to implement. *Project Schedule * until May 31 Solve architecture questions with help of community. 1 June – 30 June First, approximate implementation supporting microvacuum for GiST. I’ve got bachelor's degree in this month so I haven’t much time to work on project. 1 July – 31 July Implementation of supporting microvacuum for GiST and testing. 1 August -15 August Final refactoring, testing and committing. GSoC should be treated as a full-time job, that's how much time you're expected to dedicate to it. Having bachelor's degree exams in June would be a serious problem. You'll need to discuss with the potential mentors on how to make up for that time. Other than that, the schedule seems fairly relaxed. In fact, this project seems a bit too small for a GSoC project. I'd suggest coming up with some additional GiST-related work that you could do, in addition to the microvacuum thing. Otherwise I think there's a risk that you finish the patch in May, and have nothing to do for the rest of the summer. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Sloppy SSPI error reporting code
On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
> On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
> > On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
> > > While looking at fe-auth.c I noticed quite a few places that weren't
> > > bothering to make error messages localizable (ie, missing libpq_gettext
> > > calls), and/or were failing to add a trailing newline as expected in
> > > libpq error messages. Perhaps these are intentional but I doubt it.
> > > Most of the instances seemed to be SSPI-related.
> > >
> > > I have no intention of fixing these myself, but whoever committed that
> > > code should take a second look.
> >
> > I looked through that file and only found two cases; patch attached.
>
> Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
Oh, I accidentally saw the backend version of that function, which
looked fine. I have attached a patch for that.
> > ! printfPQExpBuffer(&conn->errorMessage,
> > libpq_gettext("SSPI returned invalid number of output buffers\n"));
> > !libpq_gettext("fe_sendauth: error
> > sending password authentication\n"));
>
> The first message is too internals-focused for a translation to be useful. If
> it were in the backend, we would use elog() and not translate. The second is
> a non-actionable message painting over a wide range of specific failures;
> translating it would just put lipstick on the pig.
OK.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 8927df4..08cc906
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*** pg_SSPI_error(PGconn *conn, const char *
*** 236,245
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
sysmsg, sizeof(sysmsg), NULL) == 0)
! printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x",
mprefix, (unsigned int) r);
else
! printfPQExpBuffer(&conn->errorMessage, "%s: %s (%x)",
mprefix, sysmsg, (unsigned int) r);
}
--- 236,245
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
sysmsg, sizeof(sysmsg), NULL) == 0)
! printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
mprefix, (unsigned int) r);
else
! printfPQExpBuffer(&conn->errorMessage, "%s: %s (%x)\n",
mprefix, sysmsg, (unsigned int) r);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assertion failure when streaming logical changes
On 10 February 2015 at 21:43, Andres Freund wrote: > On 2015-02-10 22:06:34 +0900, Michael Paquier wrote: > > On Tue, Feb 10, 2015 at 9:46 PM, Andres Freund > > wrote: > > > > > Yea, it really looks like the above commit is to "blame". The new xmin > > > tracking infrastructure doesn't know about the historical snapshot... > > > > > > > I think that we need a better regression coverage here... For example, we > > could add some tap tests in test_decoding to test streaming of logical > > changes. This would help in the future to detect such problems via the > > buildfarm. > > I agree. It's more or less a accident that the assert - which just > should be moved in the regd_count == 0 branch - didn't trigger for the > SQL interface. The snapshot acquired by the SELECT statement prevents it > there. > > It's not entirely trivial to add tests for receivelogical though. You > need to stop it programatically after a while. > > I reckon we should improve that then. What about an idle timeout for pg_recvlogical, so we can get it to time out after (say) 5 seconds of no data? Time-based facilities aren't great in tests though. The SQL interface can stop after a given number of changes received or a target LSN. Is there a practical reason pg_recvlogical doesn't? Or just that there wasn't a reason to implement it? I figured I'd ask before jumping in and trying to add it in case I'd be wasting my time trying. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Something is rotten in the state of Denmark...
On Wed, Apr 1, 2015 at 7:05 PM, Tom Lane wrote: > I've been able to reproduce this. The triggering event seems to be that > the "VACUUM FULL pg_am" in vacuum.sql has to happen while another backend > is starting up. With a ten-second delay inserted at the bottom of > PerformAuthentication(), it's trivial to hit it manually. The reason we'd > not seen this before the rolenames.sql test was added is that none of the > other tests that run concurrently with vacuum.sql perform mid-test > reconnections, or ever have AFAIR. So as long as they all managed to > start up before vacuum.sql got to the dangerous step, no problem. > > I've not fully tracked it down, but I think that the blame falls on the > MVCC-snapshots-for-catalog-scans patch; it appears that it's trying to > read pg_am's pg_class entry with a snapshot that's too old, possibly > because it assumes that sinval signaling is alive which I think ain't so. Hmm, sorry for the bug. It looks to me like sinval signaling gets started up at the beginning of InitPostgres(). Perhaps something like this? diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 7cfa0cf..47c4002 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -303,7 +303,7 @@ GetNonHistoricCatalogSnapshot(Oid relid) * Mark new snapshost as valid. We must do this last, in case an * ERROR occurs inside GetSnapshotData(). */ - CatalogSnapshotStale = false; + CatalogSnapshotStale = !IsNormalProcessingMode(); } return CatalogSnapshot; -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Stephen Frost writes: >> > REVOKE'ing access *without* removing the permissions checks would defeat >> > the intent of these changes, which is to allow an administrator to grant >> > the ability for a certain set of users to cancel and/or terminate >> > backends started by other users, without also granting those users >> > superuser rights. >> >> I see: we have two different use-cases and no way for GRANT/REVOKE >> to manage both cases using permissions on a single object. Carry >> on then. > > Alright, after going about this three or four different ways, I've > settled on the approach proposed in the attached patch. Most of it is > pretty straight-forward: drop the hard-coded check in the function > itself and REVOKE EXECUTE on the function from PUBLIC. The interesting > bits are those pieces which are more complex than the "all-or-nothing" > cases. > > This adds a few new SQL-level functions which return unfiltered results, > if you're allowed to call them based on the GRANT system (and EXECUTE > privileges for them are REVOKE'd from PUBLIC, of course). These are: > pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and > pg_signal_backend (which is similar but not the same- that allows you to > send signals to other backends as a regular user, if you are allowed to > call that function and the other backend wasn't started by a superuser). > > There are two new views added, which simply sit on top of the new > functions: pg_stat_activity_all and pg_stat_replication_all. > > Minimizing the amount of code duplication wasn't too hard with the > pg_stat_get_wal_senders case; as it was already returning a tuplestore > and so I simply moved most of the logic into a "helper" function which > handled populating the tuplestore and then each call path was relatively > small and mostly boilerplate. pg_stat_get_activity required a bit more > in the way of changes, but they were essentially to modify it to return > a tuplestore like pg_stat_get_wal_senders, and then add in the extra > function and boilerplate for the non-filtered path. > > I had considered (and spent a good bit of time implementing...) a number > of alternatives, mostly around trying to do more at the SQL level when > it came to filtering, but that got very ugly very quickly. There's no > simple way to find out "what was the effective role of the caller of > this function" from inside of a security definer function (which would > be required for the filtering, as it would need to be able to call the > function underneath). This is necessary, of course, because functions > in views run as the caller of the view, not as the view owner (that's > useful in some cases, less useful in others). I looked at exposing the > information about the effective role which was calling a function, but > that got pretty ugly too. The GUC system has a stack, but we don't > actually update the 'role' GUC when we are in security definer > functions. There's the notion of an "outer" role ID, but that doesn't > account for intermediate security definer functions and so, while it's > fine for what it does, it wasn't really helpful in this case. > > I do still think it'd be nice to provide that information and perhaps we > can do so with fmgr_security_definer(), but it's beyond what's really > needed here and I don't think it'd end up being particularly cleaner to > do it all in SQL now that I've refactored pg_stat_get_activity. > > I'd further like to see the ability to declare on either a function call > by function call basis (inside a view defintion), of even at the view > level (as that would allow me to build up multiple views with different > parameters) "who to run this function as", where the options would be > "view owner" or "view querier", very similar to our SECURITY DEFINER vs. > SECURITY INVOKER options for functions today. This is interesting > because, more and more, we're building functions which are actually > returning data sets, not individual values, and we want to filter those > sets sometimes and not other times. In any case, those are really > thoughts for the future and get away from what this is all about, which > is reducing the need for monitoring and/or "regular" admins to have the > "superuser" bit when they don't really need it. > > Clearly, further testing and documentation is required and I'll be > getting to that over the next couple of days, but it's pretty darn late > and I'm currently getting libpq undefined reference errors, probably > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > Thoughts? The tricky part of this seems to me to be the pg_dump changes. The new catalog flag seems a little sketchy to me; wouldn't it be better to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, DUMP_NONE? Is this creating a user-visible API break, where pg_stat_activity and pg_stat_replication now
Re: [HACKERS] Parallel Seq Scan
On Thu, Apr 2, 2015 at 2:36 AM, Amit Kapila wrote: >> If I'm not confused, it would be the other way around. We would run >> the initPlan in the master backend *first* and then the rest in the >> workers. > > Either one of us is confused, let me try to describe my understanding in > somewhat more detail. Let me try to explain w.r.t the tab completion > query [1]. In this, the initPlan is generated for Qualification expression > [2], so it will be executed during qualification and the callstack will > look like: > > postgres.exe!ExecSeqScan(ScanState * node=0x0c33bce8) Line 113 C > postgres.exe!ExecProcNode(PlanState * node=0x0c33bce8) Line 418 + > 0xa bytes C > postgres.exe!ExecSetParamPlan(SubPlanState * node=0x0c343930, > ExprContext * econtext=0x0c33de50) Line 1001 + 0xa bytes C >> postgres.exe!ExecEvalParamExec(ExprState * exprstate=0x0c33f980, >> ExprContext * econtext=0x0c33de50, char * isNull=0x0c33f481, >> ExprDoneCond * isDone=0x) Line C > postgres.exe!ExecMakeFunctionResultNoSets(FuncExprState * > fcache=0x0c33f0d0, ExprContext * econtext=0x0c33de50, char * > isNull=0x0042f1c8, ExprDoneCond * isDone=0x) Line > 1992 + 0x2d bytes C > postgres.exe!ExecEvalOper(FuncExprState * fcache=0x0c33f0d0, > ExprContext * econtext=0x0c33de50, char * isNull=0x0042f1c8, > ExprDoneCond * isDone=0x) Line 2443 C > postgres.exe!ExecQual(List * qual=0x0c33fa08, ExprContext * > econtext=0x0c33de50, char resultForNull=0) Line 5206 + 0x1a bytes C > postgres.exe!ExecScan(ScanState * node=0x0c33dd38, TupleTableSlot > * (ScanState *)* accessMtd=0x000140232940, char (ScanState *, > TupleTableSlot *)* recheckMtd=0x0001402329e0) Line 195 + 0x1a bytes C > postgres.exe!ExecSeqScan(ScanState * node=0x0c33dd38) Line 114 C > > Basically here initPlan is getting executed during Qualification. OK, I failed to realize that the initPlan doesn't get evaluated until first use. Maybe in the case of a funnel node we should force all of the initplans to be run before starting parallelism, so that we can pass down the resulting value to each worker. If we try to push the whole plan tree down from the worker then, aside from the issue of needing to copy the plan tree, it'll get evaluated N times instead of once. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel Seq Scan
On Thu, Apr 2, 2015 at 3:07 AM, Amit Kapila wrote: > On Wed, Apr 1, 2015 at 6:11 PM, Robert Haas wrote: >> On Wed, Apr 1, 2015 at 7:30 AM, Amit Kapila >> wrote: >> >> Also, the new code to propagate >> >> XactLastRecEnd won't work right, either. >> > >> > As we are generating FATAL error on termination of worker >> > (bgworker_die()), so won't it be handled in AbortTransaction path >> > by below code in parallel-mode patch? >> >> That will asynchronously flush the WAL, but if the master goes on to >> commit, we've wait synchronously for WAL flush, and possibly sync rep. > > Okay, so you mean if master backend later commits, then there is > a chance of loss of WAL data written by worker. > Can't we report the location to master as the patch does in case of > Commit in worker? That's exactly why I think it needs to call WaitForParallelWorkersToFinish() - because it will do just that. We only need to invent a way of telling the worker to stop the scan and shut down cleanly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On Wed, Apr 1, 2015 at 1:56 PM, Tom Lane wrote: > Andres Freund writes: >> On 2015-04-01 13:29:33 -0400, Tom Lane wrote: >>> WHEN won't help; if there are any INSTEAD OF triggers, no insert will >>> happen, whether the triggers actually fire or not. > >> Well, right now it doesn't work at all. It seems pretty reasonable to >> define things so that the insert happens normally if there's no matching >> INSTEAD OF trigger. > > It would absolutely *not* be reasonable for WHEN conditions for triggers > on tables to work completely differently than they do for triggers on > views. That ship's sailed. Clue me in, because I'm confused. If no trigger fires, we do whatever an object of that type would normally do in the absence of any trigger, no? For a view, that's error out; for a table, that's perform the action on the underlying data. That doesn't seem terribly unprincipled. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
Robert Haas writes: > On Wed, Apr 1, 2015 at 1:56 PM, Tom Lane wrote: >> It would absolutely *not* be reasonable for WHEN conditions for triggers >> on tables to work completely differently than they do for triggers on >> views. That ship's sailed. > Clue me in, because I'm confused. If no trigger fires, we do whatever > an object of that type would normally do in the absence of any > trigger, no? For a view, that's error out; for a table, that's > perform the action on the underlying data. That doesn't seem terribly > unprincipled. I dunno about unprincipled; but we have already laid down the definition of INSTEAD OF triggers, and they act as I described. Read the code if you doubt it: which path is taken in ExecInsert depends only on whether INSTEAD OF triggers *exist* on the rel, not whether any of them actually fired (indeed, it would be difficult even to know that from here). I believe this was intentional, not just a coding artifact; it stems from having wanted to throw the error for uninsertable view well upstream of here, rather than having it be conditional on what happens at runtime. What I am objecting to is Andres' claim that it would be okay for INSTEAD OF triggers on tables to act completely differently in this regard from those on views. We have laid down the definition for views, and it is that nothing happens if the trigger exists but doesn't fire. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Centralize definition of integer limits.
On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund wrote: >> > I'm tempted to just prefix our limits with PG_ and define them >> > unconditionally, including appropriate casts to our types. >> >> I don't have a better idea. > > Will push that. I'd appreciate it if you could do this soon. I like to compile with -Werror, and this problem means I can't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] possible dsm bug in dsm_attach()
On Wed, Mar 25, 2015 at 6:11 AM, Amit Kapila wrote: > I am facing an issue in case we need to create many segments for > large inheritance hierarchy. Attached patch fixes the problem for me. Sigh. You'd think I'd be able to write a 30-line patch without introducing not one but two stupid bugs. But if you did think that, you'd be wrong. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Why SyncRepWakeQueue is not static?
On Wed, Mar 25, 2015 at 10:07 PM, Tatsuo Ishii wrote: >>> Fix committed/pushed from master to 9.2. 9.1 declares it as a static >>> function. >> >> Er, is that a good idea to back-patch that? Normally routine specs are >> maintained stable on back-branches, and this is just a cosmetic >> change. > > I'm not sure if it's a cosmetic change or not. I thought declaring > to-be-static function as extern is against our coding > standard. Moreover, if someone wants to change near the place in the > source code in the future, changes made to head may not be easily back > patched or cherry-picked to older branches if I do not back patch it. True. But if any third-party code calls that function, you just broke it. I don't think keeping the back-branches consistent with master is a sufficiently good reason for such a change. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
On 04/02/2015 12:39 PM, Abhijit Menon-Sen wrote: At 2015-03-25 19:18:51 +0200, hlinn...@iki.fi wrote: I think we'll need a version check there. […] You want to write that or should I? I'm not familiar with MSVC at all, so it would be nice if you did it. Thinking more about the configure checks, I think the current approach of using inline assembly on gcc is not quite right. We're only using inline assembly to force producing SSE 4.2 code, even when -msse4.2 is not used. That feels wrong. And who's to say that the assembler supports the SSE instructions anyway? At least we'd need a configure check for that too. We have a buildfarm animal that still uses gcc 2.95.3, which was released in 2001. I don't have a compiler of that vintage to test with, but I assume an old enough assembler would not know about the crc32q instruction and fail to compile. I believe the GCC way to do this would be to put the SSE4.2-specific code into a separate source file, and compile that file with "-msse4.2". And when you compile with -msse4.2, gcc actually also supports the _mm_crc32_u8/u64 intrinsics. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
At 2015-04-02 17:58:23 +0300, hlinn...@iki.fi wrote: > > We're only using inline assembly to force producing SSE 4.2 code, even > when -msse4.2 is not used. That feels wrong. Why? It feels OK to me (and to Andres, per earlier discussions about exactly this topic). Doing it this way allows the binary to run on a non-SSE4.2 platform (and not use the CRC instructions). Also, -msse4.2 was added to the compiler later than support for the instructions was added to the assembler. > We have a buildfarm animal that still uses gcc 2.95.3, which was > released in 2001. I don't have a compiler of that vintage to test > with, but I assume an old enough assembler would not know about the > crc32q instruction and fail to compile. GCC from <2002 wouldn't support the symbolic operand names in inline assembly. binutils from <2007 (IIRC) wouldn't support the assembler instructions themselves. We could work around the latter by using the appropriate sequence of bytes. We could work around the former by using the old syntax for operands. > I believe the GCC way to do this would be to put the SSE4.2-specific > code into a separate source file, and compile that file with > "-msse4.2". And when you compile with -msse4.2, gcc actually also > supports the _mm_crc32_u8/u64 intrinsics. I have no objection to this. Building only that file with -msse4.2 would resolve the problem of the output binary requiring SSE4.2; and the only compilers to be excluded are old enough to be uninteresting at least to me personally. Have you done/are you doing this already, or do you want me to? I could use advice on how to add build flags to only one file, since I don't know of any precendent for that. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On 2 April 2015 at 14:59, Tom Lane wrote: > Robert Haas writes: >> On Wed, Apr 1, 2015 at 1:56 PM, Tom Lane wrote: >>> It would absolutely *not* be reasonable for WHEN conditions for triggers >>> on tables to work completely differently than they do for triggers on >>> views. That ship's sailed. > >> Clue me in, because I'm confused. If no trigger fires, we do whatever >> an object of that type would normally do in the absence of any >> trigger, no? For a view, that's error out; for a table, that's >> perform the action on the underlying data. That doesn't seem terribly >> unprincipled. > > I dunno about unprincipled; but we have already laid down the definition > of INSTEAD OF triggers, and they act as I described. Read the code if you > doubt it: which path is taken in ExecInsert depends only on whether > INSTEAD OF triggers *exist* on the rel, not whether any of them actually > fired (indeed, it would be difficult even to know that from here). > I believe this was intentional, not just a coding artifact; it stems from > having wanted to throw the error for uninsertable view well upstream of > here, rather than having it be conditional on what happens at runtime. > > What I am objecting to is Andres' claim that it would be okay for INSTEAD > OF triggers on tables to act completely differently in this regard from > those on views. We have laid down the definition for views, and it is > that nothing happens if the trigger exists but doesn't fire. > Well actually the fact that the code is structured that way is somewhat academic. INSTEAD OF triggers on views don't support WHEN conditions -- deliberately so, since it would be difficult to know in general what to do if the trigger didn't fire. So ExecInsert is implicitly using the existence of the trigger to imply that it will fire, although arguably it would be neater for it to double-check that, and error out if for some reason the trigger didn't fire. In any case, that doesn't establish any kind of behavioural precedent for how a conditional INSTEAD OF trigger on a table ought to work. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Centralize definition of integer limits.
On 2015-04-02 10:42:58 -0400, Robert Haas wrote: > On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund wrote: > >> > I'm tempted to just prefix our limits with PG_ and define them > >> > unconditionally, including appropriate casts to our types. > >> > >> I don't have a better idea. > > > > Will push that. > > I'd appreciate it if you could do this soon. I like to compile with > -Werror, and this problem means I can't. Done. Sorry for not doing this sooner, I'm more or less having holidays right now. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Centralize definition of integer limits.
On Thu, Apr 2, 2015 at 11:54 AM, Andres Freund wrote: > On 2015-04-02 10:42:58 -0400, Robert Haas wrote: >> On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund wrote: >> >> > I'm tempted to just prefix our limits with PG_ and define them >> >> > unconditionally, including appropriate casts to our types. >> >> >> >> I don't have a better idea. >> > >> > Will push that. >> >> I'd appreciate it if you could do this soon. I like to compile with >> -Werror, and this problem means I can't. > > Done. Sorry for not doing this sooner, I'm more or less having holidays > right now. Thanks. Sorry to interrupt your vacation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
On 2015-04-02 20:57:24 +0530, Abhijit Menon-Sen wrote: > At 2015-04-02 17:58:23 +0300, hlinn...@iki.fi wrote: > > > > We're only using inline assembly to force producing SSE 4.2 code, even > > when -msse4.2 is not used. That feels wrong. > > Why? It feels OK to me (and to Andres, per earlier discussions about > exactly this topic). Doing it this way allows the binary to run on a > non-SSE4.2 platform (and not use the CRC instructions). Right. And SSE4.2 isn't that widespread yet. > > I believe the GCC way to do this would be to put the SSE4.2-specific > > code into a separate source file, and compile that file with > > "-msse4.2". And when you compile with -msse4.2, gcc actually also > > supports the _mm_crc32_u8/u64 intrinsics. To me this seems like a somewhat pointless exercise. I actually think from a performance POV it's better to have all the functions in one source file, so the compiler can inline things into the trampoline if it feels like it. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] varlena.c hash_any() and hash_uint32() calls require DatumGetUInt32()
On Wed, Mar 25, 2015 at 9:55 AM, Peter Geoghegan wrote: > Attached patch adds DatumGetUInt32() around the hash_any() and > hash_uint32() calls within varlena.c. These should have been in the > original abbreviated keys commit. Mea culpa. Committed. Sorry for the delay; I'm still catching up from last week, when I was at PGCONF.US. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deparsing utility commands
On Wed, Mar 25, 2015 at 3:21 PM, Ryan Pedela wrote: > On Wed, Mar 25, 2015 at 11:59 AM, Alvaro Herrera > wrote: >> * Should we prohibit DDL from within event triggers? > > Please don't prohibit DDL unless there is a really, really good reason to do > so. I have several use cases in mind for event triggers, but they are only > useful if I can perform DDL. > > For example, I want to create an Elasticsearch FDW and use that to index and > search Postgres tables. When a table is created, I am planning to use an > event trigger to capture the CREATE event and automatically create a foreign > table via the Elasticsearch FDW. In addition, I would add normal triggers to > the new table which capture DML and update the foreign table accordingly. In > other words, I want to use FDWs and event triggers to automatically sync > table DDL and DML to Elasticsearch. +1. I think that if it's unsafe to run DDL from with event triggers, then that might be a sign that it's not safe to run *any* user-defined code at that location. I think it's the job of the event trigger to make sure that it doesn't assume anything about what may have changed while the user-defined code was running. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deparsing utility commands
On 2015-04-02 12:05:13 -0400, Robert Haas wrote: > On Wed, Mar 25, 2015 at 3:21 PM, Ryan Pedela wrote: > > On Wed, Mar 25, 2015 at 11:59 AM, Alvaro Herrera > > wrote: > >> * Should we prohibit DDL from within event triggers? > > > > Please don't prohibit DDL unless there is a really, really good reason to do > > so. I have several use cases in mind for event triggers, but they are only > > useful if I can perform DDL. > > > > For example, I want to create an Elasticsearch FDW and use that to index and > > search Postgres tables. When a table is created, I am planning to use an > > event trigger to capture the CREATE event and automatically create a foreign > > table via the Elasticsearch FDW. In addition, I would add normal triggers to > > the new table which capture DML and update the foreign table accordingly. In > > other words, I want to use FDWs and event triggers to automatically sync > > table DDL and DML to Elasticsearch. > > +1. I think that if it's unsafe to run DDL from with event triggers, > then that might be a sign that it's not safe to run *any* user-defined > code at that location. I think it's the job of the event trigger to > make sure that it doesn't assume anything about what may have changed > while the user-defined code was running. First of: I don't see a fundamental reason to forbid it, I think it's just simpler to analyze that way. But I'm unconvinced by that reasoning. As you know we prohibit executing DDL on some objects in *lots* of places c.f. CheckTableNotInUse(), without that imo being a sign of a more fundamental problem. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On Mon, Mar 23, 2015 at 9:46 PM, Peter Geoghegan wrote: > On Mon, Mar 23, 2015 at 2:41 PM, Andrew Gierth > wrote: >> Peter> As I said, I don't really consider that my patch is a rewrite, >> Peter> especially V4, which changes nothing substantive except removing >> Peter> 32-bit support. >> >> Well, that's a hell of an "except". > > > I guess you're right. I'm willing to go with whatever the consensus is > on that question. So I changed my mind - I now think we should support 32-bit abbreviation. You might wonder why I've changed my mind so suddenly. It's not because I started caring about 32-bit systems overnight. For the record, I still think that they are almost irrelevant. But I've seen a new angle. One of the likely future uses for abbreviated keys is to guide B-Tree index scans. One technique that looks really interesting is storing an abbreviated key in internal pages. I always knew that abbreviation is as useful for index scans as it is for sorting - maybe more so. Reading "Modern B-Tree techniques" [1] again today, I realized that we can store fixed sized abbreviated keys in line items directly. If we stored a 4 byte abbreviated key, we'd pay no storage overhead on 64-bit systems that already lose that to ItemId alignment. We'd only generate abbreviated keys in internal B-Tree pages, during relatively infrequent page splits (internal pages naturally have a very wide spread of values anyway), so that would be a very low cost. Even when abbreviation doesn't work out, we have to visit the line item anyway, and an integer comparison on the same cacheline is effectively free. It would probably work out all the time anyway, owing to the natural spread of items within internal pages. Best of all, most index scans don't even need to look past the itemId array (for internal pages, which are the large majority visited by any given index scan, but a tiny minority of those actually stored), hugely cutting down on the number of cache misses. I could see this making index scans on numeric IndexTuples noticeably faster than even those on int8 IndexTuples. Of course, text is the type that this is really compelling for (perhaps int4 too - perhaps we could automatically get this for types that fit in 4 bytes naturally on 64-bit platforms). I'm not sure that we could do this with text without adopting ICU, which makes firm guarantees about binary sort key stability, so I thought that numeric could be considered a proof of concept for that. [1] http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.219.7269&rep=rep1&type=pdf -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vac truncation scan problems
Kyotaro HORIGUCHI wrote: > Hi, this is a bug in the commit 0d831389749a3baaced7b984205b9894a82444b9 . > > It allows vucuum freeze to be skipped and inversely lets regular > vacuum wait for lock. The attched patch fixes it. > > > In table_recheck_autovac, vacuum options are determined as following, > > >tab->at_vacoptions = VACOPT_SKIPTOAST | > >(dovacuum ? VACOPT_VACUUM : 0) | > >(doanalyze ? VACOPT_ANALYZE : 0) | > !>(wraparound ? VACOPT_NOWAIT : 0); > > The line prefixed by '!' looks inverted. You're absolutely right. My mistake. Pushed your patch, thanks. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] POLA violation with \c service=
David Fetter wrote: > I haven't checked yet, but could this be because people aren't using > --enable-depend with ./configure ? BTW --- No, this can't be the answer; --enable-depend is meant to help with recompiling after updating the source tree, but lack of it cannot cause any failures (assuming proper usage of make distclean). -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
Robert Haas writes: > On Wed, Apr 1, 2015 at 7:05 PM, Tom Lane wrote: >> I've not fully tracked it down, but I think that the blame falls on the >> MVCC-snapshots-for-catalog-scans patch; it appears that it's trying to >> read pg_am's pg_class entry with a snapshot that's too old, possibly >> because it assumes that sinval signaling is alive which I think ain't so. > Hmm, sorry for the bug. It looks to me like sinval signaling gets > started up at the beginning of InitPostgres(). Perhaps something like > this? Yeah, it does, so you'd think this would work. I've now tracked down exactly what's happening, and it's this: while we're reading pg_authid during PerformAuthentication, we establish a catalog snapshot. Later on, we read pg_database to find out what DB we're connecting to. If any sinval messages for unshared system catalogs arrive at that point, they'll effectively be ignored, *because our MyDatabaseId is still zero and so doesn't match the incoming message*. That's okay as far as the catcaches are concerned, cause they're empty, and the relcache only knows about shared catalogs so far. But InvalidateCatalogSnapshot doesn't get called by LocalExecuteInvalidationMessage, and so we think we can plow ahead with the old snapshot. It looks to me like an appropriate fix would be as attached; thoughts? (I've tested this with a delay during relation lock acquisition and concurrent whole-database VACUUM FULLs, and so far been unable to break it.) regards, tom lane diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 1e646a1..6aa6868 100644 *** a/src/backend/utils/init/postinit.c --- b/src/backend/utils/init/postinit.c *** InitPostgres(const char *in_dbname, Oid *** 853,858 --- 853,866 MyProc->databaseId = MyDatabaseId; /* + * We may have already established a catalog snapshot while trying to read + * pg_authid; but until we have set up MyDatabaseId, we won't react to + * incoming sinval messages properly, so we won't realize it if the + * snapshot has been invalidated. Best to assume it's no good anymore. + */ + InvalidateCatalogSnapshot(); + + /* * Now, take a writer's lock on the database we are trying to connect to. * If there is a concurrently running DROP DATABASE on that database, this * will block us until it finishes (and has committed its update of -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vac truncation scan problems
Alvaro Herrera writes: > Kyotaro HORIGUCHI wrote: >> The line prefixed by '!' looks inverted. > You're absolutely right. My mistake. Pushed your patch, thanks. Don't see any such commit from here? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > > Clearly, further testing and documentation is required and I'll be > > getting to that over the next couple of days, but it's pretty darn late > > and I'm currently getting libpq undefined reference errors, probably > > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > > > Thoughts? > > The tricky part of this seems to me to be the pg_dump changes. The > new catalog flag seems a little sketchy to me; wouldn't it be better > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, > DUMP_NONE? I agree that the pg_dump changes are a very big part of this change. I'll look at using an enum and see if that would work. > Is this creating a user-visible API break, where pg_stat_activity and > pg_stat_replication now always show only the publicly-visible > information, and privileged users must explicitly decide to query the > _all versions? If so, -1 from me on that part of this. Thanks for bringing it up. No, the existing API is exactly the same for the existing views- the only difference is that now there are _all versions which provide unfiltered data (but you have to have permission to call the _all() functions underneath, or you get a permission denied error). Where this does have an API change is with the simpler functions that used to do "if (superuser() || replication_role())" and now depend on the GRANT system instead. We can't (today, at least) say: GRANT EXECUTE ON function_whatever() TO replication_roles; And have that be kept current as that role attribute is modified. I've not done it yet, but we could certainly have pg_dump dump out GRANTs for the *current* users which have that role attribute and give those users access to the functions via the normal permissions system. I'm not really sure that's a good idea though- it might be better to have a clean break here (and one which is clearly in the "more secure/explicit" direction) and tell admins in the release notes to GRANT EXECUTE on the functions for the roles they want to give permission to. We could also duplicate the functions and have the existing ones remain as-is and have the new ones just depend on the GRANT system, but I don't particularly like that since I'm afraid we'd end up in the same boat we're in now with pg_user and friends. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] POLA violation with \c service=
On 04/02/2015 12:42 PM, Alvaro Herrera wrote: David Fetter wrote: I haven't checked yet, but could this be because people aren't using --enable-depend with ./configure ? BTW --- No, this can't be the answer; --enable-depend is meant to help with recompiling after updating the source tree, but lack of it cannot cause any failures (assuming proper usage of make distclean). And the buildfarm always builds with pristine sources. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On Tue, Mar 24, 2015 at 3:03 AM, Andrew Gierth
wrote:
> So here's the latest (and, hopefully, last) version:
>
> - adds diagnostic output from numeric_abbrev_abort using the trace_sort
>GUC
>
> - fixed Datum cs. uint32 issues in hash_uint32
>
> - added a short comment about excess-k representation
>
> - tweaked the indenting and comments a bit
>
> I'm not particularly committed to any specific way of handling the
> DEC_DIGITS issue. (I moved away from the "transparently skip
> abbreviations" approach of the original because it seemed that reducing
> #ifdefism in the code was a desirable feature.)
>
> The INT64_MIN/MAX changes should be committed fairly soon. (I haven't
> posted a patch for TRACE_SORT)
I think this is really nice work, so I have committed this version. I
made a few fairly minor changes, hopefully without breaking anything
in the process:
- I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
- I removed (void) ssup; which I do not think is normal PostgreSQL style.
- Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
think we don't need protect against #if 0 code get re-enabled.
- I removed the too-clever (at least IMHO) handing of TRACE_SORT in
favor of just using #ifdef around each occurrence.
- I also declared trace_sort in guc.h, where various other GUCs are
declared, instead of declaring it privately in each file that needed
it.
- Changed some definitions to depend on SIZEOF_DATUM rather than
USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
- Fixed an OID conflict.
- And of course, bumped catversion.
Thanks,
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Zero-padding and zero-masking fixes for to_char(float)
On Wed, Apr 1, 2015 at 11:48:37AM -0400, Bruce Momjian wrote:
> This should return something like 16.000... (per Oracle
> output at the URL above, float4 has 6 significant digits on my compiler)
> but I can't seem to figure how to get printf() to round non-fractional
> parts. I am afraid the only solution is to use printf's %e format and
> place the decimal point myself.
Hearing nothing, I went with the %e approach; patch attached. The new
output looks right:
test=> SELECT to_char(float4
'15.9123456789123456789000',
repeat('9', 50) || '.' || repeat('9', 50));
to_char
16.00
(1 row)
> The fact I still don't have a complete solution suggests this is 9.6
> material but I still want to work on it so it is ready.
I will keep this patch for 9.6 unless I hear otherwise.
--
Bruce Momjian http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
new file mode 100644
index 40a353f..d283cd2
*** a/src/backend/utils/adt/formatting.c
--- b/src/backend/utils/adt/formatting.c
***
*** 113,125
#define DCH_MAX_ITEM_SIZ 12 /* max localized day name */
#define NUM_MAX_ITEM_SIZ 8 /* roman number (RN has 15 chars) */
- /* --
- * More is in float.c
- * --
- */
- #define MAXFLOATWIDTH 60
- #define MAXDOUBLEWIDTH 500
-
/* --
* Format parser structs
--- 113,118
*** static DCHCacheEntry *DCH_cache_getnew(c
*** 989,994
--- 982,989
static NUMCacheEntry *NUM_cache_search(char *str);
static NUMCacheEntry *NUM_cache_getnew(char *str);
static void NUM_cache_remove(NUMCacheEntry *ent);
+ static char *add_pre_zero_padding(char *num_str, int decimal_shift);
+ static char *add_post_zero_padding(char *num_str, int pad_digits);
/* --
*** do { \
*** 5016,5021
--- 5011,5103
SET_VARSIZE(result, len + VARHDRSZ); \
} while (0)
+ /*
+ * add_pre_zero_padding
+ *
+ * printf() can only round non-fractional parts of a number if the number
+ * is in exponential format, so this function takes a number in that format
+ * and adds pre-decimal zero padding.
+ */
+ static char *
+ add_pre_zero_padding(char *num_str, int decimal_shift)
+ {
+ /* one for decimal point, one for trailing null byte */
+ char *out = palloc(strlen(num_str) + decimal_shift + 1 + 1), *out_p = out;
+ char *num_str_p = num_str;
+ bool decimal_found = false;
+
+ /* copy the number before 'e' and shift the decimal point */
+ while (*num_str_p && *num_str_p != 'e')
+ {
+ if (*num_str_p == '.')
+ {
+ num_str_p++;
+ decimal_found = true;
+ }
+ else
+ {
+ *(out_p++) = *(num_str_p++);
+ if (decimal_found)
+ decimal_shift--;
+ }
+ }
+
+ /* add zero pad digits */
+ while (decimal_shift-- > 0)
+ *(out_p++) = '0';
+
+ *(out_p++) = '\0';
+
+ pfree(num_str);
+ return out;
+ }
+
+ /*
+ * add_post_zero_padding
+ *
+ * Some sprintf() implementations have a 512-digit precision limit, and we
+ * need sprintf() to round to the internal precision, so this function adds
+ * zero padding between the mantissa and exponent of an exponential-format
+ * number, or after the supplied string for non-exponent strings.
+ */
+ static char *
+ add_post_zero_padding(char *num_str, int pad_digits)
+ {
+ /* one for decimal point, one for trailing null byte */
+ char *out = palloc(strlen(num_str) + pad_digits + 1 + 1), *out_p = out;
+ char *num_str_p = num_str;
+ bool decimal_found = false;
+
+ /* copy the number before 'e', or the entire string if no 'e' */
+ while (*num_str_p && *num_str_p != 'e')
+ {
+ if (*num_str_p == '.')
+ decimal_found = true;
+ *(out_p++) = *(num_str_p++);
+ }
+
+ /* add zero pad digits */
+ while (pad_digits-- > 0)
+ {
+ if (!decimal_found)
+ {
+ *(out_p++) = '.';
+ decimal_found = true;
+ }
+
+ *(out_p++) = '0';
+ }
+
+ /* copy 'e' and everything after */
+ while (*num_str_p)
+ *(out_p++) = *(num_str_p++);
+
+ *(out_p++) = '\0';
+
+ pfree(num_str);
+ return out;
+ }
+
/* ---
* NUMERIC to_number() (convert string to numeric)
* ---
*** int4_to_char(PG_FUNCTION_ARGS)
*** 5214,5221
/* we can do it easily because float8 won't lose any precision */
float8 val = (float8) value;
! orgnum = (char *) palloc(MAXDOUBLEWIDTH + 1);
! snprintf(orgnum, MAXDOUBLEWIDTH + 1, "%+.*e", Num.post, val);
/*
* Swap a leading positive sign for a space.
--- 5296,5307
/
Re: [HACKERS] Something is rotten in the state of Denmark...
On Thu, Apr 2, 2015 at 12:54 PM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Apr 1, 2015 at 7:05 PM, Tom Lane wrote: >>> I've not fully tracked it down, but I think that the blame falls on the >>> MVCC-snapshots-for-catalog-scans patch; it appears that it's trying to >>> read pg_am's pg_class entry with a snapshot that's too old, possibly >>> because it assumes that sinval signaling is alive which I think ain't so. > >> Hmm, sorry for the bug. It looks to me like sinval signaling gets >> started up at the beginning of InitPostgres(). Perhaps something like >> this? > > Yeah, it does, so you'd think this would work. I've now tracked down > exactly what's happening, and it's this: while we're reading pg_authid > during PerformAuthentication, we establish a catalog snapshot. Later on, > we read pg_database to find out what DB we're connecting to. If any > sinval messages for unshared system catalogs arrive at that point, they'll > effectively be ignored, *because our MyDatabaseId is still zero and so > doesn't match the incoming message*. That's okay as far as the catcaches > are concerned, cause they're empty, and the relcache only knows about > shared catalogs so far. But InvalidateCatalogSnapshot doesn't get called > by LocalExecuteInvalidationMessage, and so we think we can plow ahead with > the old snapshot. > > It looks to me like an appropriate fix would be as attached; thoughts? > (I've tested this with a delay during relation lock acquisition and > concurrent whole-database VACUUM FULLs, and so far been unable to break > it.) Hmm, that fix doesn't reach as far as what I did. My proposal would regard a catalog snapshot as immediately stale, so if we're asked for a catalog snapshot multiple times before InitPostgres() is called, we'll take a new one every time. Your proposal invalidates them just once, after setting up MyDatabaseId. Assuming yours is safe, it's better, because it invalidates less aggressively. The only thing I'm worried about is that I think PerformAuthentication() runs before InitPostgres(); sinval isn't working at all at that point, even for shared catalogs. If we were to lock a relation, build a relcache entry, unlock it, and then do that again, all before SharedInvalBackendInit(false) is called, what would keep us from failing to notice an intervening invalidation? Maybe there is something, because otherwise this was probably broken even before the MVCC-catalog-snapshots patch, but I don't know what it is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [HACKERS] GSoC 2015 proposal: Improve the performance of “ALTER TABLE .. SET LOGGED / UNLOGGED” statement
On Wed, Mar 25, 2015 at 9:46 PM, Fabrízio de Royes Mello wrote: > http://www.postgresql.org/message-id/ca+tgmozm+-0r7h0edpzzjbokvvq+gavkchmno4fypveccw-...@mail.gmail.com I like the idea of the feature a lot, but the proposal to which you refer here mentions some problems, which I'm curious how you think you might solve. (I don't have any good ideas myself, beyond what I mentioned there.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Remove fsync ON/OFF as a visible option?
On Wed, Mar 25, 2015 at 3:45 PM, Jim Nasby wrote: > I see 3 settings that allow people to accidentally shoot themselves in the > foot; fsync, wal_sync_method and full_page_writes. Those aren't even the top three in my experience, let alone the only three. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix pgbench --progress report under (very) low rate
On Sun, Mar 22, 2015 at 6:12 AM, Fabien COELHO wrote: > When running with low rate, the --progress is only printed when there is > some activity, which makes it quite irregular, including some catching up > with stupid tps figures. > > Shame on me for this "feature" (aka bug) in the first place. > > This patch fixes this behavior by considering the next report time as a > target to meet as well as transaction schedule times. > > Before the patch: > > sh> ./pgbench -R 0.5 -T 10 -P 1 -S > progress: 1.7 s, 0.6 tps, lat 6.028 ms stddev -nan, lag 1.883 ms > progress: 2.2 s, 2.3 tps, lat 2.059 ms stddev -nan, lag 0.530 ms > progress: 7.3 s, 0.4 tps, lat 2.944 ms stddev 1.192, lag 2.624 ms > progress: 7.3 s, 1402.5 tps, lat 5.115 ms stddev 0.000, lag 0.000 ms > progress: 7.3 s, 0.0 tps, lat -nan ms stddev -nan, lag inf ms > progress: 7.3 s, 335.2 tps, lat 3.106 ms stddev 0.000, lag 0.000 ms > progress: 8.8 s, 0.0 tps, lat -nan ms stddev -nan, lag inf ms > progress: 8.8 s, 307.6 tps, lat 4.855 ms stddev -nan, lag 0.000 ms > progress: 10.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms > > After the patch: > > sh> ./pgbench -R 0.5 -T 10 -P 1 -S > progress: 1.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms > progress: 2.0 s, 1.0 tps, lat 5.980 ms stddev 0.000, lag 0.733 ms > progress: 3.0 s, 1.0 tps, lat 1.905 ms stddev 0.000, lag 0.935 ms > progress: 4.0 s, 1.0 tps, lat 3.966 ms stddev 0.000, lag 0.623 ms > progress: 5.0 s, 2.0 tps, lat 2.527 ms stddev 1.579, lag 0.512 ms > progress: 6.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms > progress: 7.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms > progress: 8.0 s, 1.0 tps, lat 1.750 ms stddev 0.000, lag 0.767 ms > progress: 9.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms > progress: 10.0 s, 2.0 tps, lat 2.403 ms stddev 1.386, lag 0.357 ms I haven't had time to really review the code here (except to notice that you have a typo: "nedded") but the idea of it seems good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Resetting crash time of background worker
On Sun, Mar 22, 2015 at 10:55 PM, Amit Khandekar wrote: > On 17 March 2015 at 19:12, Robert Haas wrote: >> On Tue, Mar 17, 2015 at 1:33 AM, Amit Khandekar >> wrote: >> > I think we either have to retain the knowledge that the worker has >> > crashed >> > using some new field, or else, we should reset the crash time only if it >> > is >> > not flagged BGW_NEVER_RESTART. >> >> I think you're right, and I think we should do the second of those. >> Thanks for tracking this down. > > Thanks. Attached a patch accordingly. Put this into the June 2015 > commitfest. Committed and back-patched to 9.4. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
Robert Haas writes: > On Thu, Apr 2, 2015 at 12:54 PM, Tom Lane wrote: >> It looks to me like an appropriate fix would be as attached; thoughts? > Hmm, that fix doesn't reach as far as what I did. My proposal would > regard a catalog snapshot as immediately stale, so if we're asked for > a catalog snapshot multiple times before InitPostgres() is called, > we'll take a new one every time. Your proposal invalidates them just > once, after setting up MyDatabaseId. Assuming yours is safe, it's > better, because it invalidates less aggressively. Right. > The only thing I'm worried about is that I think > PerformAuthentication() runs before InitPostgres(); sinval isn't > working at all at that point, even for shared catalogs. No, PerformAuthentication is called by InitPostgres. However, I'm having second thoughts about whether we've fully diagnosed this. Three out of the four failures we've seen in the buildfarm reported "cache lookup failed for access method 403", not "could not open relation with OID 2601" ... and I'm so far only able to replicate the latter symptom. It's really unclear how the former one could arise, because nothing that vacuum.sql does would change xmin of the rows in pg_am. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
On Thu, Apr 2, 2015 at 2:40 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Apr 2, 2015 at 12:54 PM, Tom Lane wrote: >>> It looks to me like an appropriate fix would be as attached; thoughts? > >> Hmm, that fix doesn't reach as far as what I did. My proposal would >> regard a catalog snapshot as immediately stale, so if we're asked for >> a catalog snapshot multiple times before InitPostgres() is called, >> we'll take a new one every time. Your proposal invalidates them just >> once, after setting up MyDatabaseId. Assuming yours is safe, it's >> better, because it invalidates less aggressively. > > Right. > >> The only thing I'm worried about is that I think >> PerformAuthentication() runs before InitPostgres(); sinval isn't >> working at all at that point, even for shared catalogs. > > No, PerformAuthentication is called by InitPostgres. Oops, OK. > However, I'm having second thoughts about whether we've fully diagnosed > this. Three out of the four failures we've seen in the buildfarm reported > "cache lookup failed for access method 403", not "could not open relation > with OID 2601" ... and I'm so far only able to replicate the latter > symptom. It's really unclear how the former one could arise, because > nothing that vacuum.sql does would change xmin of the rows in pg_am. It probably changes the *relfilenode* of pg_am, because it runs VACUUM FULL on that catalog. Perhaps some backend sees the old relfilenode value and tries to a heap scan, interpreting the now-truncated file as empty? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
Robert Haas writes: > On Thu, Apr 2, 2015 at 2:40 PM, Tom Lane wrote: >> However, I'm having second thoughts about whether we've fully diagnosed >> this. Three out of the four failures we've seen in the buildfarm reported >> "cache lookup failed for access method 403", not "could not open relation >> with OID 2601" ... and I'm so far only able to replicate the latter >> symptom. It's really unclear how the former one could arise, because >> nothing that vacuum.sql does would change xmin of the rows in pg_am. > It probably changes the *relfilenode* of pg_am, because it runs VACUUM > FULL on that catalog. Perhaps some backend sees the old relfilenode > value and tries to a heap scan, interpreting the now-truncated file as > empty? Yeah, I came up with the same theory a few minutes later. Trying to reproduce on that basis. Actually, now that I think it through, the "could not open relation" error is pretty odd in itself. If we are trying to open pg_am using a stale catalog snapshot, it seems like we ought to reliably find its old pg_class tuple (the one with the obsolete relfilenode), rather than finding nothing. But the latter is the behavior I'm seeing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort
On Fri, Feb 20, 2015 at 3:57 PM, Tomas Vondra wrote: > I've spent a fair amount of testing this today, and when using the > simple percentile_disc example mentioned above, I see this pattern: > > master patched speedup >- > generate_series(1,100) 4.2 0.7 6 > generate_series(1,200) 9.2 9.8 0.93 > generate_series(1,300) 14.5 15.3 0.95 > > > so for a small dataset the speedup is very nice, but for larger sets > there's ~5% slowdown. Is this expected? I had a look at this patch today with a view to committing it, but it seems that nobody's commented on this point, which seems like an important one. Any thoughts? For what it's worth, and without wishing to provoke another flamewar, I am inclined to use Andrew's version of this patch rather than Peter's. I have not undertaken an exhaustive comparison, nor do I intend to. It is the reviewer's responsibility to justify the changes they think the author needs to make, and that wasn't done here. On the points of difference Andrew highlighted, I think his version is fine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
On Thu, Apr 2, 2015 at 2:55 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Apr 2, 2015 at 2:40 PM, Tom Lane wrote: >>> However, I'm having second thoughts about whether we've fully diagnosed >>> this. Three out of the four failures we've seen in the buildfarm reported >>> "cache lookup failed for access method 403", not "could not open relation >>> with OID 2601" ... and I'm so far only able to replicate the latter >>> symptom. It's really unclear how the former one could arise, because >>> nothing that vacuum.sql does would change xmin of the rows in pg_am. > >> It probably changes the *relfilenode* of pg_am, because it runs VACUUM >> FULL on that catalog. Perhaps some backend sees the old relfilenode >> value and tries to a heap scan, interpreting the now-truncated file as >> empty? > > Yeah, I came up with the same theory a few minutes later. Trying to > reproduce on that basis. > > Actually, now that I think it through, the "could not open relation" > error is pretty odd in itself. If we are trying to open pg_am using > a stale catalog snapshot, it seems like we ought to reliably find its > old pg_class tuple (the one with the obsolete relfilenode), rather than > finding nothing. But the latter is the behavior I'm seeing. What's to stop the old tuple from being HOT-pruned? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas wrote:
> I think this is really nice work, so I have committed this version. I
> made a few fairly minor changes, hopefully without breaking anything
> in the process:
>
> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
> think we don't need protect against #if 0 code get re-enabled.
> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
> favor of just using #ifdef around each occurrence.
> - I also declared trace_sort in guc.h, where various other GUCs are
> declared, instead of declaring it privately in each file that needed
> it.
> - Changed some definitions to depend on SIZEOF_DATUM rather than
> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
> - Fixed an OID conflict.
> - And of course, bumped catversion.
And that's broken the buildfarm. Argh.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
Robert Haas writes: > On Thu, Apr 2, 2015 at 2:55 PM, Tom Lane wrote: >> Actually, now that I think it through, the "could not open relation" >> error is pretty odd in itself. If we are trying to open pg_am using >> a stale catalog snapshot, it seems like we ought to reliably find its >> old pg_class tuple (the one with the obsolete relfilenode), rather than >> finding nothing. But the latter is the behavior I'm seeing. > What's to stop the old tuple from being HOT-pruned? Hm, that may be it. I went back to the previous test scenario, and now I can *only* get the "cache lookup failed for access method" behavior, instead of what I was getting before, so I'm getting a bit confused :-(. However, it does seem clear that the mechanism is indeed that we're relying on an obsolete copy of pg_am's pg_class tuple, hence scanning a truncated relfilenode, and that the patch I proposed fixes it. Perhaps the difference has to do with whether pg_am's pg_class tuple is on a page that hasn't got enough room for a HOT update? But I definitely tried it several times and consistently got the same failure before. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
On Thu, Apr 2, 2015 at 3:59 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Apr 2, 2015 at 2:55 PM, Tom Lane wrote: >>> Actually, now that I think it through, the "could not open relation" >>> error is pretty odd in itself. If we are trying to open pg_am using >>> a stale catalog snapshot, it seems like we ought to reliably find its >>> old pg_class tuple (the one with the obsolete relfilenode), rather than >>> finding nothing. But the latter is the behavior I'm seeing. > >> What's to stop the old tuple from being HOT-pruned? > > Hm, that may be it. I went back to the previous test scenario, and now > I can *only* get the "cache lookup failed for access method" behavior, > instead of what I was getting before, so I'm getting a bit confused :-(. > However, it does seem clear that the mechanism is indeed that we're > relying on an obsolete copy of pg_am's pg_class tuple, hence scanning a > truncated relfilenode, and that the patch I proposed fixes it. > > Perhaps the difference has to do with whether pg_am's pg_class tuple is > on a page that hasn't got enough room for a HOT update? But I definitely > tried it several times and consistently got the same failure before. That seems plausible, except that I have no idea why that would vary from one test setup to another. I suggest pushing the patch you proposed upthread and seeing what the buildfarm thinks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Something is rotten in the state of Denmark...
Robert Haas writes: > On Thu, Apr 2, 2015 at 3:59 PM, Tom Lane wrote: >> Perhaps the difference has to do with whether pg_am's pg_class tuple is >> on a page that hasn't got enough room for a HOT update? But I definitely >> tried it several times and consistently got the same failure before. > That seems plausible, except that I have no idea why that would vary > from one test setup to another. I suggest pushing the patch you > proposed upthread and seeing what the buildfarm thinks. Yeah. I have errands to do right now but will push it later. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On 02/04/15 21:21, Robert Haas wrote:
On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas wrote:
I think this is really nice work, so I have committed this version. I
made a few fairly minor changes, hopefully without breaking anything
in the process:
- I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
- I removed (void) ssup; which I do not think is normal PostgreSQL style.
- Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
think we don't need protect against #if 0 code get re-enabled.
- I removed the too-clever (at least IMHO) handing of TRACE_SORT in
favor of just using #ifdef around each occurrence.
- I also declared trace_sort in guc.h, where various other GUCs are
declared, instead of declaring it privately in each file that needed
it.
- Changed some definitions to depend on SIZEOF_DATUM rather than
USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
- Fixed an OID conflict.
- And of course, bumped catversion.
And that's broken the buildfarm. Argh.
That's because of this:
+#ifdef SIZEOF_DATUM == 8
You need just #if there.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On Thu, Apr 2, 2015 at 4:21 PM, Petr Jelinek wrote:
> On 02/04/15 21:21, Robert Haas wrote:
>> On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas wrote:
>>>
>>> I think this is really nice work, so I have committed this version. I
>>> made a few fairly minor changes, hopefully without breaking anything
>>> in the process:
>>>
>>> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
>>> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
>>> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
>>> think we don't need protect against #if 0 code get re-enabled.
>>> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
>>> favor of just using #ifdef around each occurrence.
>>> - I also declared trace_sort in guc.h, where various other GUCs are
>>> declared, instead of declaring it privately in each file that needed
>>> it.
>>> - Changed some definitions to depend on SIZEOF_DATUM rather than
>>> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
>>> - Fixed an OID conflict.
>>> - And of course, bumped catversion.
>>
>>
>> And that's broken the buildfarm. Argh.
>
> That's because of this:
>
> +#ifdef SIZEOF_DATUM == 8
>
> You need just #if there.
Thanks. I actually pushed a fix for that about 25 minutes ago;
hopefully that is all that is needed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On 02/04/15 22:22, Robert Haas wrote:
On Thu, Apr 2, 2015 at 4:21 PM, Petr Jelinek wrote:
On 02/04/15 21:21, Robert Haas wrote:
On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas wrote:
I think this is really nice work, so I have committed this version. I
made a few fairly minor changes, hopefully without breaking anything
in the process:
- I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
- I removed (void) ssup; which I do not think is normal PostgreSQL style.
- Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
think we don't need protect against #if 0 code get re-enabled.
- I removed the too-clever (at least IMHO) handing of TRACE_SORT in
favor of just using #ifdef around each occurrence.
- I also declared trace_sort in guc.h, where various other GUCs are
declared, instead of declaring it privately in each file that needed
it.
- Changed some definitions to depend on SIZEOF_DATUM rather than
USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
- Fixed an OID conflict.
- And of course, bumped catversion.
And that's broken the buildfarm. Argh.
That's because of this:
+#ifdef SIZEOF_DATUM == 8
You need just #if there.
Thanks. I actually pushed a fix for that about 25 minutes ago;
hopefully that is all that is needed.
Ok, the git.pg.org was somewhat behind. It did fix it for me when I
tested it locally.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] improve pgbench syntax error messages
On Sun, Mar 29, 2015 at 2:20 PM, Fabien COELHO wrote: >> Here is a v5. > > Here is v6, just a rebase. Committed with minor stylistic fixes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
On Thu, Apr 2, 2015 at 4:24 PM, Petr Jelinek wrote: >> Thanks. I actually pushed a fix for that about 25 minutes ago; >> hopefully that is all that is needed. > > Ok, the git.pg.org was somewhat behind. It did fix it for me when I tested > it locally. OK, that's good to know. So far the buildfarm looks happy too, but I'll try to keep an eye on it at least for the next hour or so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq's multi-threaded SSL callback handling is busted
On 4/2/15 4:32 AM, Jan Urbański wrote: > > Peter Eisentraut writes: > >> On 2/12/15 7:28 AM, Jan Urbański wrote: >>> For the sake of discussion, here's a patch to prevent stomping on >>> previously-set callbacks, racy as it looks. >>> >>> FWIW, it does fix the Python deadlock and doesn't cause the PHP segfault... >> >> I don't think this patch would actually fix the problem that was >> described after the original bug report >> (http://www.postgresql.org/message-id/5436991b.5020...@vmware.com), >> namely that another thread acquires a lock while the libpq callbacks are >> set and then cannot release the lock if libpq has been shut down in the >> meantime. > > I did test both the Python and the PHP repro scripts and the patch fixed both > the deadlock and the segfault. > > What happens is that Python (for instance) stops over the callback > unconditionally. So when libpq gets unloaded, it sees that the currently set > callback is no the one it originally set and refrains from NULLing it. So this works because Python is just as broken as libpq right now. What happens if Python is fixed as well? Then we'll have the problem I described above. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] extend pgbench expressions with functions
On Sun, Mar 29, 2015 at 2:20 PM, Fabien COELHO wrote: >> Here is a small v2 update: > > v3, just a rebase. Thanks for working on this. I see it's already registered in the 2015-06 CF, and will have a look at when we get there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On 4/2/15 11:50 AM, Dean Rasheed wrote: > Well actually the fact that the code is structured that way is > somewhat academic. INSTEAD OF triggers on views don't support WHEN > conditions -- deliberately so, since it would be difficult to know in > general what to do if the trigger didn't fire. So ExecInsert is > implicitly using the existence of the trigger to imply that it will > fire, although arguably it would be neater for it to double-check > that, and error out if for some reason the trigger didn't fire. In any > case, that doesn't establish any kind of behavioural precedent for how > a conditional INSTEAD OF trigger on a table ought to work. I think the upshot is that INSTEAD OF triggers work in a particular way because that's what is needed to support updatable views. If triggers on tables should behave differently, maybe it should be a separate trigger type. Maybe it would be feasible to extend BEFORE triggers to support RETURNING, for example? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Table-level log_autovacuum_min_duration
Michael Paquier wrote:
> So, attached are two patches:
> - 0001 enables SIGHUP tracking in do_autovacuum(), which is checked
> before processing one table. I reused avl_sighup_handler for the
> worker, renaming it av_sighup_handler..
> - 0002 is the patch to add log_autovacuum_min_duration as a reloption.
> There is nothing really changed, the value of
> log_autovacuum_min_duration being picked up at startup with
> table_recheck_autovac() is used until the end for one table.
Thanks.
You added this in the worker loop processing each table:
/*
* Check for config changes before processing each collected
table.
*/
if (got_SIGHUP)
{
got_SIGHUP = false;
ProcessConfigFile(PGC_SIGHUP);
/* shutdown requested in config file? */
if (!AutoVacuumingActive())
break;
}
I think bailing out if autovac is disabled is a good idea; for instance,
if this is a for-wraparound vacuum, we should keep running. My first
reaction is that this part of the test ought to be moved down a
screenful or so, until we've ran the recheck step and we have the
for-wraparound flag in hand; only bail out if that one is not set. But
on the other hand, maybe the worker will process some tables that are
not for-wraparound in between some other tables that are, so that might
turn out to be a bad idea as well. (Though I'm not 100% that it works
that way; consider commit f51ead09df for instance.) Maybe the test to
use for this is something along the lines of "if autovacuum was enabled
before and is no longer enabled, bail out, otherwise keep running".
This implies that we need to keep track of whether autovac was enabled
at the start of the worker run.
Another thing, but not directly related to this patch: while looking at
the doc changes, I noticed that we don't have index entries for the
reloptions we have; for instance, the word "fillfactor" doesn't appear
in the bookindex.html page at all. Having these things all crammed in
the CREATE TABLE page seems somewhat poor. I think we could improve on
this by having a listing of settable parameters in a separate section,
and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter
link to that; we could also have index entries for these items.
Of course, the simpler fix is to just add a few tags.
As a note, there is no point to "Assert(foo != NULL)" tests when foo is
later dereferenced in the same routine: the crash is going to happen
anyway at the dereference, so it's just a LOC uselessly wasted.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On 2015-04-02 16:42:43 -0400, Peter Eisentraut wrote: > On 4/2/15 11:50 AM, Dean Rasheed wrote: > > Well actually the fact that the code is structured that way is > > somewhat academic. INSTEAD OF triggers on views don't support WHEN > > conditions -- deliberately so, since it would be difficult to know in > > general what to do if the trigger didn't fire. So ExecInsert is > > implicitly using the existence of the trigger to imply that it will > > fire, although arguably it would be neater for it to double-check > > that, and error out if for some reason the trigger didn't fire. In any > > case, that doesn't establish any kind of behavioural precedent for how > > a conditional INSTEAD OF trigger on a table ought to work. > > I think the upshot is that INSTEAD OF triggers work in a particular way > because that's what is needed to support updatable views. If triggers > on tables should behave differently, maybe it should be a separate > trigger type. Maybe it would be feasible to extend BEFORE triggers to > support RETURNING, for example? What in the above prohibits extending the behaviour to tables? I have yet to see what compatibility or similarity problem that'd pose. It seems all mightily handwavy to me. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On Thu, Apr 2, 2015 at 5:02 PM, Andres Freund wrote: >> I think the upshot is that INSTEAD OF triggers work in a particular way >> because that's what is needed to support updatable views. If triggers >> on tables should behave differently, maybe it should be a separate >> trigger type. Maybe it would be feasible to extend BEFORE triggers to >> support RETURNING, for example? > > What in the above prohibits extending the behaviour to tables? I have > yet to see what compatibility or similarity problem that'd pose. It > seems all mightily handwavy to me. Yeah. It's possible there's a better interface here than INSTEAD OF, and one of the things I didn't like about the OP was that it started by stating the syntax that would be used rather than by describing the problem that needed to be solved. It's generally better to start with the latter, and then work out the syntax from there. But having gotten that gripe out of my system, and especially in view of Dean's comments, it's not very clear to me what's wrong with using INSTEAD OF for this purpose. If you make BEFORE triggers do this via RETURNING, then you might have a trigger that returns multiple rows, which seems like it would introduce a bunch of new complexity for no obvious benefit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort
On Thu, Apr 2, 2015 at 8:17 PM, Robert Haas wrote: > On Fri, Feb 20, 2015 at 3:57 PM, Tomas Vondra > wrote: >> I've spent a fair amount of testing this today, and when using the >> simple percentile_disc example mentioned above, I see this pattern: >> >> master patched speedup >>- >> generate_series(1,100) 4.2 0.7 6 >> generate_series(1,200) 9.2 9.8 0.93 >> generate_series(1,300) 14.5 15.3 0.95 >> >> >> so for a small dataset the speedup is very nice, but for larger sets >> there's ~5% slowdown. Is this expected? I think it's explained by the pre-check for sorted input making the number of comparisons exactly n -1. As I pointed out to Tomas, if you put a single, solitary unsorted element at the end, the abbreviated version is then 8x faster (maybe that was in relation to a slightly different case, but the pattern is the same). So this case isn't an argument against datum abbreviation, or even abbreviation in general, but rather an argument against using strxfrm() in general (which for example the GCC docs strongly recommend for sorting lists of strings). It's a bad argument, IMV. This sort is already extremely fast. BTW, Corey Huinker (who I believe you also spoke to during pgConf.US) reported that his problematic CREATE INDEX case was 12x faster with 9.5. That used tapesort. That was also perfectly pre-sorted, but since it was using tapesort and abbreviated there was a huge improvement. > I had a look at this patch today with a view to committing it, but it > seems that nobody's commented on this point, which seems like an > important one. Any thoughts? > > For what it's worth, and without wishing to provoke another flamewar, > I am inclined to use Andrew's version of this patch rather than > Peter's. I have not undertaken an exhaustive comparison, nor do I > intend to. It is the reviewer's responsibility to justify the changes > they think the author needs to make, and that wasn't done here. On > the points of difference Andrew highlighted, I think his version is > fine. The justification is that Andrew's version had arbitrary differences with the existing code, in particular in the datum comparator, which was different to all other cases in the way that Andrew pointed out. Overall, there were only tiny differences between the two versions. I see no reason to not match the existing style. The changes that Andrew took issue with are utterly insignificant. Also, the changes that Andrew didn't mention are clearly appropriate. In particular, the comments on the SortKeys variable being used by every case except the hash case and datum case should still be updated to reflect that that's only true for the hash case now. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort
On Thu, Apr 2, 2015 at 5:34 PM, Peter Geoghegan wrote: > I think it's explained by the pre-check for sorted input making the > number of comparisons exactly n -1. As I pointed out to Tomas, if you > put a single, solitary unsorted element at the end, the abbreviated > version is then 8x faster (maybe that was in relation to a slightly > different case, but the pattern is the same). So this case isn't an > argument against datum abbreviation, or even abbreviation in general, > but rather an argument against using strxfrm() in general (which for > example the GCC docs strongly recommend for sorting lists of strings). > It's a bad argument, IMV. This sort is already extremely fast. OK, I see. > The changes that Andrew > took issue with are utterly insignificant. Great. Then you will be utterly indifferent to which version gets committed. > Also, the changes that Andrew didn't mention are clearly appropriate. > In particular, the comments on the SortKeys variable being used by > every case except the hash case and datum case should still be updated > to reflect that that's only true for the hash case now. On that point, I agree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
On Thu, Apr 2, 2015 at 5:33 PM, Heikki Linnakangas wrote: > It was added in gcc 4.2. That's good enough for me. I think it's fine to have optional optimizations that require gcc >= 4.2, as long as older platforms don't break outright. >>> We have a buildfarm animal that still uses gcc 2.95.3, which was >>> released in 2001. I don't have a compiler of that vintage to test >>> with, but I assume an old enough assembler would not know about the >>> crc32q instruction and fail to compile. >> >> >> GCC from <2002 wouldn't support the symbolic operand names in inline >> assembly. binutils from <2007 (IIRC) wouldn't support the assembler >> instructions themselves. >> >> We could work around the latter by using the appropriate sequence of >> bytes. We could work around the former by using the old syntax for >> operands. > > I'm OK with not supporting the new instructions when building with an old > compiler/assembler. But the build shouldn't fail with an old > compiler/assembler. Using old syntax or raw bytes just to avoid failing on > an ancient compiler seems ugly. I dunno about old syntax, but raw bytes seems like a bad idea, for sure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort
On Thu, Apr 2, 2015 at 11:21 PM, Robert Haas wrote: >> The changes that Andrew >> took issue with are utterly insignificant. > > Great. Then you will be utterly indifferent to which version gets committed. *shrug* You were the one that taught me to be bureaucratically minded about keeping code consistent at this fine a level. I think it's odd that you of all people are opposing me on this point, but whatever. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Abbreviated keys for Datum tuplesort
On Thu, Apr 2, 2015 at 11:21 PM, Robert Haas wrote: > On Thu, Apr 2, 2015 at 5:34 PM, Peter Geoghegan wrote: >> I think it's explained by the pre-check for sorted input making the >> number of comparisons exactly n -1. As I pointed out to Tomas, if you >> put a single, solitary unsorted element at the end, the abbreviated >> version is then 8x faster (maybe that was in relation to a slightly >> different case, but the pattern is the same). So this case isn't an >> argument against datum abbreviation, or even abbreviation in general, >> but rather an argument against using strxfrm() in general (which for >> example the GCC docs strongly recommend for sorting lists of strings). >> It's a bad argument, IMV. This sort is already extremely fast. > > OK, I see. Wait. It's also due to the fact that some cases are spuriously aborted, because the cost model for text needs to be fixed, I believe. It's not clear that this is actually such a case from Tomas' remarks, because the smaller scale tested *did* win significantly, and that appeared to be otherwise comparable to the regressed cases. So although what I describe here about perfectly pre-sorted input is something I've seen (and something that we discussed on another thread IIRC), this is not an example of it. Again, this is just an example of why we need to fix the cost model for text. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Unused variable in hashpage.c
Hi,
my compiler complains about unused variable nblkno in _hash_splitbucket
in no-assert build. It looks like relic of commit ed9cc2b5d which
removed the only use of that variable besides the Assert.
Looking at the code:
nblkno = start_nblkno;
Assert(nblkno == BufferGetBlockNumber(nbuf));
I was originally thinking of simply changing the Assert to use
start_nblkno but then I noticed that the start_nblkno is actually not
used anywhere in the function either (it's one of input parameters). And
given that the only caller of that function is getting the variable it
sends as nbuf to that function via start_nblkno, I think the Assert is
somewhat pointless there anyway.
So best way to solve this seems to be removing the start_nblkno from the
_hash_splitbucket altogether. Attached patch does just that.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 9a77945..2f82b1e 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -39,7 +39,6 @@ static bool _hash_alloc_buckets(Relation rel, BlockNumber firstblock,
static void _hash_splitbucket(Relation rel, Buffer metabuf,
Buffer nbuf,
Bucket obucket, Bucket nbucket,
- BlockNumber start_oblkno,
BlockNumber start_nblkno,
uint32 maxbucket,
uint32 highmask, uint32 lowmask);
@@ -666,8 +665,7 @@ _hash_expandtable(Relation rel, Buffer metabuf)
/* Relocate records to the new bucket */
_hash_splitbucket(rel, metabuf, buf_nblkno,
old_bucket, new_bucket,
- start_oblkno, start_nblkno,
- maxbucket, highmask, lowmask);
+ start_oblkno, maxbucket, highmask, lowmask);
/* Release bucket locks, allowing others to access them */
_hash_droplock(rel, start_oblkno, HASH_EXCLUSIVE);
@@ -758,13 +756,11 @@ _hash_splitbucket(Relation rel,
Bucket obucket,
Bucket nbucket,
BlockNumber start_oblkno,
- BlockNumber start_nblkno,
uint32 maxbucket,
uint32 highmask,
uint32 lowmask)
{
BlockNumber oblkno;
- BlockNumber nblkno;
Buffer obuf;
Page opage;
Page npage;
@@ -781,8 +777,6 @@ _hash_splitbucket(Relation rel,
opage = BufferGetPage(obuf);
oopaque = (HashPageOpaque) PageGetSpecialPointer(opage);
- nblkno = start_nblkno;
- Assert(nblkno == BufferGetBlockNumber(nbuf));
npage = BufferGetPage(nbuf);
/* initialize the new bucket's primary page */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Table-level log_autovacuum_min_duration
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -881,9 +881,11 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
UNLOGGED ] TABLE [ IF NOT EXI
toast., which can be used to control the behavior of the
table's secondary TOAST table, if any
(see for more information about TOAST).
-Note that the TOAST table inherits the
-autovacuum_* values from its parent table, if there are
-no toast.autovacuum_* settings set.
+Note that the TOAST table inherits values of
autovacuum_*
+and log_autovacuum_min_duration from its parent table,
if
+there are no values set for respectively
+toast.autovacuum_* and
+toast.log_autovacuum_min_duration.
I think this could use some wordsmithing; I didn't like listing the
parameters explicitely, and Jaime Casanova suggested not using the terms
"inherit" and "parent table" in this context. So I came up with this:
Note that the TOAST table uses the parameter values defined for the main
table, for each parameter applicable to TOAST tables and for which no
value is set in the TOAST table itself.
Thoughts?
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
At 2015-04-03 00:33:10 +0300, hlinn...@iki.fi wrote: > > I came up with the attached. I like it very much. src/port/Makefile has (note src/srv): +# pg_crc32c_sse42.o and its _src.o version need CFLAGS_SSE42 +pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42) +pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42) Other than that, this looks great. Thank you. > BTW, we might want to move the "traditional" and "legacy" crc32 > implementations out of src/common. They are only used in backend > code now. I agree. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Abbreviated keys for Numeric
> "Robert" == Robert Haas writes:
Robert> I think this is really nice work, so I have committed this
Robert> version. I made a few fairly minor changes, hopefully without
Robert> breaking anything in the process:
Robert> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
Robert> - I removed (void) ssup; which I do not think is normal PostgreSQL
style.
Robert> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
Robert> think we don't need protect against #if 0 code get re-enabled.
No complaints in principle for any of these
Robert> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
Robert> favor of just using #ifdef around each occurrence.
My idea here - which I have admittedly not got round to posting a patch
for yet - was that TRACE_SORT (which has been defined by default since
8.1) should be deprecated in favour of unconditional use of trace_sort.
(The actual definition of TRACE_SORT could be kept in case any extension
or whatever uses it)
Robert> - I also declared trace_sort in guc.h, where various other GUCs
Robert> are declared, instead of declaring it privately in each file
Robert> that needed it.
I was thinking miscadmin.h but whatever.
Robert> - Changed some definitions to depend on SIZEOF_DATUM rather
Robert> than USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please
Robert> check it.
No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the
definitions of Int64GetDatum / DatumGetInt64 are conditional on. As you
have it now, it'll break if you build with --disable-float8-byval on a
64bit platform.
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] The return value of allocate_recordbuf()
On Thu, Feb 12, 2015 at 4:02 PM, Michael Paquier wrote: > On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas wrote: >> >> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier >> wrote: >> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao >> > wrote: >> >> MemoryContextAllocExtended() was added, so isn't it time to replace >> >> palloc() >> >> with MemoryContextAllocExtended(CurrentMemoryContext, >> >> MCXT_ALLOC_NO_OOM) >> >> in allocate_recordbuf()? >> > >> > Yeah, let's move on here, but not with this patch I am afraid as this >> > breaks any client utility using xlogreader.c in frontend, like >> > pg_xlogdump or pg_rewind because MemoryContextAllocExtended is not >> > available in frontend, only in backend. We are going to need something >> > like palloc_noerror, defined on both backend (mcxt.c) and frontend >> > (fe_memutils.c) side. >> >> Unfortunately, that gets us back into the exact terminological dispute >> that we were hoping to avoid. Perhaps we could compromise on >> palloc_extended(). > > > Yes, why not using palloc_extended instead of palloc_noerror that has been > clearly rejected in the other thread. Now, for palloc_extended we should > copy the flags of MemoryContextAllocExtended to fe_memutils.h and have the > same interface between frontend and backend (note that MCXT_ALLOC_HUGE has > no real utility for frontends). Attached is a patch to achieve that, > completed with a second patch for the problem related to this thread. Note > that in the second patch I have added an ERROR in logical.c after calling > XLogReaderAllocate, this was missing.. The first patch looks good to me basically. But I have one comment: shouldn't we expose pg_malloc_extended as a global function like we did pg_malloc? Some frontends might need to use it in the future. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] The return value of allocate_recordbuf()
On Fri, Apr 3, 2015 at 12:56 PM, Fujii Masao wrote:
> The first patch looks good to me basically. But I have one comment:
> shouldn't we expose pg_malloc_extended as a global function like
> we did pg_malloc? Some frontends might need to use it in the future.
Yes, it makes sense as the other functions do it too. So I refactored
the patch and defined a new static inline routine,
pg_malloc_internal(), that all the other functions call to reduce the
temperature in this code path that I suppose can become quite hot even
for frontends. In the second patch I added as well what was needed for
pg_rewind.
--
Michael
From f43fc9a5a5e0ffb246782010b8860829b8304593 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 3 Apr 2015 14:21:12 +0900
Subject: [PATCH 1/2] Add palloc_extended and pg_malloc_extended for frontend
and backend
This interface can be used to control at a lower level memory allocation
using an interface similar to MemoryContextAllocExtended, but this time
applied to CurrentMemoryContext on backend side, while on frontend side
a similar interface is available.
---
src/backend/utils/mmgr/mcxt.c| 37 ++
src/common/fe_memutils.c | 43 ++--
src/include/common/fe_memutils.h | 11 ++
src/include/utils/palloc.h | 1 +
4 files changed, 81 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e2fbfd4..c42a6b6 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -864,6 +864,43 @@ palloc0(Size size)
return ret;
}
+void *
+palloc_extended(Size size, int flags)
+{
+ /* duplicates MemoryContextAllocExtended to avoid increased overhead */
+ void *ret;
+
+ AssertArg(MemoryContextIsValid(CurrentMemoryContext));
+ AssertNotInCriticalSection(CurrentMemoryContext);
+
+ if (((flags & MCXT_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) ||
+ ((flags & MCXT_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size)))
+ elog(ERROR, "invalid memory alloc request size %zu", size);
+
+ CurrentMemoryContext->isReset = false;
+
+ ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext, size);
+ if (ret == NULL)
+ {
+ if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+ {
+ MemoryContextStats(TopMemoryContext);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed on request of size %zu.", size)));
+ }
+ return NULL;
+ }
+
+ VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
+
+ if ((flags & MCXT_ALLOC_ZERO) != 0)
+ MemSetAligned(ret, 0, size);
+
+ return ret;
+}
+
/*
* pfree
* Release an allocated chunk.
diff --git a/src/common/fe_memutils.c b/src/common/fe_memutils.c
index 345221e..527f9c8 100644
--- a/src/common/fe_memutils.c
+++ b/src/common/fe_memutils.c
@@ -19,8 +19,8 @@
#include "postgres_fe.h"
-void *
-pg_malloc(size_t size)
+static inline void *
+pg_malloc_internal(size_t size, int flags)
{
void *tmp;
@@ -28,22 +28,37 @@ pg_malloc(size_t size)
if (size == 0)
size = 1;
tmp = malloc(size);
- if (!tmp)
+ if (tmp == NULL)
{
- fprintf(stderr, _("out of memory\n"));
- exit(EXIT_FAILURE);
+ if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+ {
+ fprintf(stderr, _("out of memory\n"));
+ exit(EXIT_FAILURE);
+ }
+ return NULL;
}
+
+ if ((flags & MCXT_ALLOC_ZERO) != 0)
+ MemSet(tmp, 0, size);
return tmp;
}
void *
+pg_malloc(size_t size)
+{
+ return pg_malloc_internal(size, 0);
+}
+
+void *
pg_malloc0(size_t size)
{
- void *tmp;
+ return pg_malloc_internal(size, MCXT_ALLOC_ZERO);
+}
- tmp = pg_malloc(size);
- MemSet(tmp, 0, size);
- return tmp;
+void *
+pg_malloc_extended(size_t size, int flags)
+{
+ return pg_malloc_internal(size, flags);
}
void *
@@ -100,13 +115,19 @@ pg_free(void *ptr)
void *
palloc(Size size)
{
- return pg_malloc(size);
+ return pg_malloc_internal(size, 0);
}
void *
palloc0(Size size)
{
- return pg_malloc0(size);
+ return pg_malloc_internal(size, MCXT_ALLOC_ZERO);
+}
+
+void *
+palloc_extended(Size size, int flags)
+{
+ return pg_malloc_internal(size, flags);
}
void
diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index db7cb3e..6466167 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -9,10 +9,20 @@
#ifndef FE_MEMUTILS_H
#define FE_MEMUTILS_H
+/*
+ * Flags for pg_malloc_extended and palloc_extended, deliberately named
+ * the same as the backend flags.
+ */
+#define MCXT_ALLOC_HUGE 0x01 /* allow huge allocation (> 1 GB)
+ * not actually used for frontends */
+#define MCXT_ALLOC_NO_OOM 0x02 /* no failure if out-of-memory */
+#define MCXT_ALLOC_ZERO 0x04 /* zero allocated memory */
+
/* "Safe" memory allocation functions --- these exit(1) on failure */
extern char *pg_strdup(const char *in);
extern void *pg_malloc(size_t size);
extern void *pg_malloc0(size_t size);
+extern void *pg_malloc_extended(size_t size
Re: [HACKERS] Supporting TAP tests with MSVC and Windows
Each Windows patch should cover all Windows build systems; patches that fix a
problem in the src/tools/msvc build while leaving it broken in the GNU make
build are a bad trend. In this case, I expect you'll need few additional
changes to cover both.
On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:
> 2) tapcheck does not check if IPC::Run is installed or not. Should we
> add a check in the code for that? If yes, should we bypass the test or
> fail?
The src/tools/msvc build system officially requires ActivePerl, and I seem to
recall ActivePerl installs IPC::Run by default. A check is optional.
> 7) TAP tests use sometimes top_builddir directly. I think that's ugly,
> and if there are no objections I think that we should change it to use
> a dedicated environment variable like TESTTOP or similar.
I sense nothing ugly about a top_builddir reference, but see below.
> --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
> +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
> open HBA, ">>$tempdir/pgdata/pg_hba.conf";
> -print HBA "local replication all trust\n";
> +# Windows builds do not support local connections
> +if ($Config{osname} ne "MSWin32")
> +{
> + print HBA "local replication all trust\n";
> +}
> print HBA "host replication all 127.0.0.1/32 trust\n";
> print HBA "host replication all ::1/128 trust\n";
> close HBA;
Test suites that run as part of "make check-world", including all src/bin TAP
suites, _must not_ enable trust authentication on Windows. To do so would
reintroduce CVE-2014-0067. (The standard alternative is to call "pg_regress
--config-auth", which switches a data directory to SSPI authentication.)
Suites not in check-world, though not obligated to follow that rule, will have
a brighter future if they do so anyway.
> --- a/src/test/perl/TestLib.pm
> +++ b/src/test/perl/TestLib.pm
> @@ -73,9 +74,19 @@ sub tempdir_short
> sub standard_initdb
> {
> my $pgdata = shift;
> - system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
> - system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
> -'--config-auth', $pgdata);
> +
> + if ($Config{osname} eq "MSWin32")
> + {
> + system_or_bail("initdb -D $pgdata -A trust -N > NUL");
> + system_or_bail("$ENV{TESTREGRESS}",
> +'--config-auth', $pgdata);
> + }
> + else
> + {
> + system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
> + system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
> +'--config-auth', $pgdata);
> + }
> }
Make all build systems set TESTREGRESS, and use it unconditionally.
That's not a complete review, just some highlights.
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Table-level log_autovacuum_min_duration
On Fri, Apr 3, 2015 at 5:57 AM, Alvaro Herrera wrote:
> You added this in the worker loop processing each table:
>
> /*
> * Check for config changes before processing each collected
> table.
> */
> if (got_SIGHUP)
> {
> got_SIGHUP = false;
> ProcessConfigFile(PGC_SIGHUP);
>
> /* shutdown requested in config file? */
> if (!AutoVacuumingActive())
> break;
> }
>
> I think bailing out if autovac is disabled is a good idea; for instance,
> if this is a for-wraparound vacuum, we should keep running. My first
> reaction is that this part of the test ought to be moved down a
> screenful or so, until we've ran the recheck step and we have the
> for-wraparound flag in hand; only bail out if that one is not set. But
> on the other hand, maybe the worker will process some tables that are
> not for-wraparound in between some other tables that are, so that might
> turn out to be a bad idea as well. (Though I'm not 100% that it works
> that way; consider commit f51ead09df for instance.) Maybe the test to
> use for this is something along the lines of "if autovacuum was enabled
> before and is no longer enabled, bail out, otherwise keep running".
> This implies that we need to keep track of whether autovac was enabled
> at the start of the worker run.
I did consider the case of wraparound but came to the conclusion that
bailing out as fast as possible was the idea. But well, I guess that
we could play it safe and not add this check after all. The main
use-case of this change is now to be able to do re-balancing of the
cost parameters. We could still decide later if a worker could bail
out earlier or not, depending on what.
> Another thing, but not directly related to this patch: while looking at
> the doc changes, I noticed that we don't have index entries for the
> reloptions we have; for instance, the word "fillfactor" doesn't appear
> in the bookindex.html page at all. Having these things all crammed in
> the CREATE TABLE page seems somewhat poor. I think we could improve on
> this by having a listing of settable parameters in a separate section,
> and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter
> link to that; we could also have index entries for these items.
> Of course, the simpler fix is to just add a few tags.
This sounds like a good idea, and this refactoring would meritate a
different patch and a different thread. I guess that it should be a
new section in Server Configuration then, named "Relation Options",
with two different subsections for index options and table options.
> As a note, there is no point to "Assert(foo != NULL)" tests when foo is
> later dereferenced in the same routine: the crash is going to happen
> anyway at the dereference, so it's just a LOC uselessly wasted.
Check. I still think that it is useful in this case though... But removed.
> I think this could use some wordsmithing; I didn't like listing the
> parameters explicitely, and Jaime Casanova suggested not using the terms
> "inherit" and "parent table" in this context. So I came up with this:
> Note that the TOAST table uses the parameter values defined for the main
> table, for each parameter applicable to TOAST tables and for which no
> value is set in the TOAST table itself.
Fine for me.
--
Michael
From f43fc9a5a5e0ffb246782010b8860829b8304593 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 3 Apr 2015 14:21:12 +0900
Subject: [PATCH 1/2] Add palloc_extended and pg_malloc_extended for frontend
and backend
This interface can be used to control at a lower level memory allocation
using an interface similar to MemoryContextAllocExtended, but this time
applied to CurrentMemoryContext on backend side, while on frontend side
a similar interface is available.
---
src/backend/utils/mmgr/mcxt.c| 37 ++
src/common/fe_memutils.c | 43 ++--
src/include/common/fe_memutils.h | 11 ++
src/include/utils/palloc.h | 1 +
4 files changed, 81 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e2fbfd4..c42a6b6 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -864,6 +864,43 @@ palloc0(Size size)
return ret;
}
+void *
+palloc_extended(Size size, int flags)
+{
+ /* duplicates MemoryContextAllocExtended to avoid increased overhead */
+ void *ret;
+
+ AssertArg(MemoryContextIsValid(CurrentMemoryContext));
+ AssertNotInCriticalSection(CurrentMemoryContext);
+
+ if (((flags & MCXT_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) ||
+ ((flags & MCXT_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size)))
+ elog(ERROR, "invalid memory alloc request size %zu", size);
+
+ CurrentMemoryContext->isRe
Re: [HACKERS] Supporting TAP tests with MSVC and Windows
Thanks for your input, Noah.
On Fri, Apr 3, 2015 at 3:22 PM, Noah Misch wrote:
> Each Windows patch should cover all Windows build systems; patches that fix a
> problem in the src/tools/msvc build while leaving it broken in the GNU make
> build are a bad trend. In this case, I expect you'll need few additional
> changes to cover both.
Yes I am planning to do more tests with MinGW as well, once I got the
patch in a more advanced state in May/June. For Cygwin, I am not much
familiar with it yet, but I am sure I'll come up with something.
> On Thu, Apr 02, 2015 at 06:30:02PM +0900, Michael Paquier wrote:
>> 2) tapcheck does not check if IPC::Run is installed or not. Should we
>> add a check in the code for that? If yes, should we bypass the test or
>> fail?
>
> The src/tools/msvc build system officially requires ActivePerl, and I seem to
> recall ActivePerl installs IPC::Run by default. A check is optional.
I recall installing ActivePerl for my own box some time ago. It is
5.16, so now it is outdated, but the package manager does not contain
IPC::Run and I had to install it by hand.
> Test suites that run as part of "make check-world", including all src/bin TAP
> suites, _must not_ enable trust authentication on Windows. To do so would
> reintroduce CVE-2014-0067. (The standard alternative is to call "pg_regress
> --config-auth", which switches a data directory to SSPI authentication.)
> Suites not in check-world, though not obligated to follow that rule, will have
> a brighter future if they do so anyway.
OK. I'll rework this part.
>> --- a/src/test/perl/TestLib.pm
>> +++ b/src/test/perl/TestLib.pm
>
>> @@ -73,9 +74,19 @@ sub tempdir_short
>> sub standard_initdb
>> {
>> my $pgdata = shift;
>> - system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
>> - system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
>> -'--config-auth', $pgdata);
>> +
>> + if ($Config{osname} eq "MSWin32")
>> + {
>> + system_or_bail("initdb -D $pgdata -A trust -N > NUL");
>> + system_or_bail("$ENV{TESTREGRESS}",
>> +'--config-auth', $pgdata);
>> + }
>> + else
>> + {
>> + system_or_bail("initdb -D '$pgdata' -A trust -N >/dev/null");
>> +
>> system_or_bail("$ENV{top_builddir}/src/test/regress/pg_regress",
>> +'--config-auth', $pgdata);
>> + }
>> }
>
> Make all build systems set TESTREGRESS, and use it unconditionally.
Yeah, that would be better.
> That's not a complete review, just some highlights.
Thanks again.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tables cannot have INSTEAD OF triggers
On 2 April 2015 at 22:23, Robert Haas wrote: > On Thu, Apr 2, 2015 at 5:02 PM, Andres Freund wrote: >>> I think the upshot is that INSTEAD OF triggers work in a particular way >>> because that's what is needed to support updatable views. If triggers >>> on tables should behave differently, maybe it should be a separate >>> trigger type. Maybe it would be feasible to extend BEFORE triggers to >>> support RETURNING, for example? >> >> What in the above prohibits extending the behaviour to tables? I have >> yet to see what compatibility or similarity problem that'd pose. It >> seems all mightily handwavy to me. > > Yeah. It's possible there's a better interface here than INSTEAD OF, > and one of the things I didn't like about the OP was that it started > by stating the syntax that would be used rather than by describing the > problem that needed to be solved. It's generally better to start with > the latter, and then work out the syntax from there. But having > gotten that gripe out of my system, and especially in view of Dean's > comments, it's not very clear to me what's wrong with using INSTEAD OF > for this purpose. If you make BEFORE triggers do this via RETURNING, > then you might have a trigger that returns multiple rows, which seems > like it would introduce a bunch of new complexity for no obvious > benefit. > Yes, I'm inclined to agree. One of the reasons that INSTEAD OF triggers weren't supported on tables was the lack of an obvious use-case for it, but now having thought about partitioning, I think they would provide a fairly neat solution to that problem. I don't think that putting a view with INSTEAD OF triggers on top of the parent table and then always going through that view works quite as well, because there are still a few cases where a view doesn't work as well as a table. A view can't be the target of a foreign key, for example, so there'd be no way for a cascaded UPDATE to invoke the INSTEAD OF triggers. If you needed to handle the case of updates causing a change of partition, adding conditional INSTEAD OF triggers to the child tables would be a way to do that, retaining support for RETURNING, and keeping the logic localised to each partition, only invoking it when necessary. Supporting INSTEAD OF triggers on tables is not completely trivial to implement, but it doesn't look too hard either, and the more I think about it, the more I suspect that other use-cases will emerge to make that effort worthwhile. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Possibly a typo in expand_inherited_rtentry()
Hi, Attached does: Index childRTindex; AppendRelInfo *appinfo; -/* Open rel if needed; we already have required locks */ +/* Open rel if needed; we already have acquired locks */ if (childOID != parentOID) newrelation = heap_open(childOID, NoLock); else Does that make sense? Thanks, Amit diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 51b3da2..b0cb818 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1317,7 +1317,7 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti) Index childRTindex; AppendRelInfo *appinfo; - /* Open rel if needed; we already have required locks */ + /* Open rel if needed; we already have acquired locks */ if (childOID != parentOID) newrelation = heap_open(childOID, NoLock); else -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
