Re: mysql_fdw crash

[066ce286]

Hi,

>Seems some basic mistake I think it should as below
>(*param_types)[i] = exprType(param_expr);
>
>After this it works


Seems to work fine from my side.

Thank you very much, it'd painful for me to find the bug, I've been too far 
away from C coding for a too long time :-(

-- 
Hervé LEFEBVRE

Re: [HACKERS] proposal: schema variables

[Pavel Stehule]

Hi

just rebase


Regards

Pavel


schema-variables-20181121-01.patch.gz
Description: application/gzip

Re: incorrect xlog.c coverage report

[Masahiko Sawada]

On Wed, Nov 21, 2018 at 12:50 AM Alvaro Herrera
 wrote:
>
> I noticed some strangeness in the test coverage reporting.  For example,
> in
> https://coverage.postgresql.org/src/backend/access/transam/xlog.c.gcov.html
> in the function readRecoveryCommandFile(), most of the branches parsing
> the individual recovery options (recovery_target_xid,
> recovery_target_time, etc.) are shown as never hit, even though there
> are explicit tests for this in
> src/test/recovery/t/003_recovery_targets.pl.  I tried this locally and
> also with -O0 just in case, but I get the same results.  Any ideas?
>

I've looked into this issue and this happens on my environment (CentOS
6.9 and gcob 4.4.7) as well. ISTM the cause would related to the
immediate shutdown mode we're using in test_recovery_standby.
Interestingly in my environment with the attached one-line patch the
coverage reports the branches parsing the individual recovery options
correctly.

If my investigation is correct, all tests where use an immediate
shutdown might not counted by gcov.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


gcov.patch
Description: Binary data

Re: A WalSnd issue related to state WALSNDSTATE_STOPPING

[Michael Paquier]

On Wed, Nov 21, 2018 at 12:54:30PM +0800, Paul Guo wrote:
> The panic reason is that since there is just one wal sender and
> WalSndCtl->walsnds[0].state is WALSNDSTATE_STOPPING so syncWalSnd will be
> NULL and that causes assert failure. Latest postgresql code removes the
> Assert code but the related similar code logic was kept. It looks like that
> we need to modify the logic similar like below (PG 9.4 STABLE) to allow
> WALSNDSTATE_STOPPING which is reasonable here If I understand correctly.

The checkpointer initializes a shutdown checkpoint where it tells to all
the WAL senders to stop once all the children processes are gone, so it
seems to me that there is little point in processing
SyncRepReleaseWaiters() when a WAL sender is in WALSNDSTATE_STOPPING 
state as at this stage syncrep makes little sense.  It is still
necessary to process standby messages at this stage so as the primary
can shut down when it is sure that all the standbys have flushed the
shutdown checkpoint record of the primary.
--
Michael


signature.asc
Description: PGP signature

Re: Continue work on changes to recovery.conf API

[Michael Paquier]

On Tue, Nov 20, 2018 at 12:07:12PM +0100, Peter Eisentraut wrote:
> I went over the patch and did a bunch of small refinements.  I have also
> updated the documentation more extensively.  The attached patch is
> committable to me.

Thanks for putting energy into that.

> Recovery is now initiated by a file recovery.signal.  Standby mode is
> initiated by a file standby.signal.  The standby_mode setting is
> gone.  If a recovery.conf file is found, an error is issued.

I am having second thoughts about this part of the patch actually.
What's bad in keeping standby_mode and just rely on recovery.signal to
enforce recovery to happen?  When the startup process starts all the
parameters should be loaded.  That would also need less work from users
to switch to the new APIs.  I think that there could be a point to
*merge* hot_standby and standby_mode actually into an enum, so keeping
standby_mode would help with that (not this patch work of course).  The
barrier between recovery.trigger standby.trigger is also rather thin. 

> The trigger_file setting has been renamed to promote_trigger_file as
> part of the move.

This rename looks fine.

> pg_basebackup -R now appends settings to postgresql.auto.conf and
> creates a standby.signal file.
> 
> Author: Simon Riggs 
> Author: Abhijit Menon-Sen 
> Author: Sergei Kornilov 

I think that Fujii Masao should also be listed as an author, or at least
get a mention.  He was one of the first to work on this stuff.

Using separate GUCs for each target is fine by me.  I would have thought
that 003_recovery_targets.pl needed some tweaks, so I am happy to see
that it is not the case.

So overall this stuff looks fine per its shape, just the part for
standby.signal may not justify breaking compatibility more than
necessary...  The first mention of this part comes from
https://postgr.es/m/canp8+jloysdjb5ip7wynvcckoe4oynhabunb8pqmgbjgfkq...@mail.gmail.com
as far as I know.
--
Michael


signature.asc
Description: PGP signature

Re: row filtering for logical replication

[Hironobu SUZUKI]

On 2018/11/01 0:29, Euler Taveira wrote:

Em qua, 28 de fev de 2018 às 20:03, Euler Taveira
 escreveu:

The attached patches add support for filtering rows in the publisher.


I rebased the patch. I added row filtering for initial
synchronization, pg_dump support and psql support. 0001 removes unused
code. 0002 reduces memory use. 0003 passes only structure member that
is used in create_estate_for_relation. 0004 reuses a parser node for
row filtering. 0005 is the feature. 0006 prints WHERE expression in
psql. 0007 adds pg_dump support. 0008 is only for debug purposes (I'm
not sure some of these messages will be part of the final patch).
0001, 0002, 0003 and 0008 are not mandatory for this feature.

Comments?




Hi,

I reviewed your patches and I found a bug when I tested ALTER 
PUBLICATION statement.


In short, ALTER PUBLICATION SET with a WHERE clause does not applied new 
WHERE clause.


I describe the outline of the test I did and my conclusion.

[TEST]
I show the test case I tried in below.

(1)Publisher and Subscriber

I executed each statement on the publisher and the subscriber.

```
testdb=# CREATE PUBLICATION pub_testdb_t FOR TABLE t WHERE (id > 10);
CREATE PUBLICATION
```

```
testdb=# CREATE SUBSCRIPTION sub_testdb_t CONNECTION 'dbname=testdb 
port=5432 user=postgres' PUBLICATION pub_testdb_t;

NOTICE:  created replication slot "sub_testdb_t" on publisher
CREATE SUBSCRIPTION
```

(2)Publisher

I executed these statements shown below.

testdb=# INSERT INTO t VALUES (1,1);
INSERT 0 1
testdb=# INSERT INTO t VALUES (11,11);
INSERT 0 1

(3)Subscriber

I confirmed that the CREATE PUBLICATION statement worked well.

```
testdb=# SELECT * FROM t;
 id | data
+--
 11 |   11
(1 row)
```

(4)Publisher
After that, I executed ALTER PUBLICATION with a WHERE clause and 
inserted a new row.


```
testdb=# ALTER  PUBLICATION pub_testdb_t SET TABLE t WHERE (id > 5);
ALTER PUBLICATION

testdb=# INSERT INTO t VALUES (7,7);
INSERT 0 1

testdb=# SELECT * FROM t;
 id | data
+--
  1 |1
 11 |   11
  7 |7
(3 rows)
```

(5)Subscriber
I confirmed that the change of WHERE clause set by ALTER PUBLICATION 
statement was ignored.


```
testdb=# SELECT * FROM t;
 id | data
+--
 11 |   11
(1 row)
```

[Conclusion]
I think AlterPublicationTables()@publicationcmds.c has a bug.

In the foreach(oldlc, oldrelids) loop, oldrel must be appended to 
delrels if oldrel or newrel has a WHERE clause. However, the current 
implementation does not, therefore, old WHERE clause is not deleted and 
the new WHERE clause is ignored.


This is my speculation. It may not be correct, but , at least, it is a 
fact that ALTER PUBLICATION with a WHERE clause is not functioned in my 
environment and my operation described in above.



Best regards,

Re: 64-bit hash function for hstore and citext data type

[amul sul]

On Wed, Nov 21, 2018 at 10:34 AM Hironobu SUZUKI  wrote:
>
> On 2018/09/26 11:20, amul sul wrote:
> > Hi all,
> >
> > Commit[1] has added 64-bit hash functions for core data types and in the 
> > same
> > discussion thread[2] Robert Haas suggested to have the similar extended hash
> > function for hstore and citext data type. Attaching patch proposes the same.
> >
> > Regards,
> > Amul
> >
> > 1] 
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=81c5e46c490e2426db243eada186995da5bb0ba7
> > 2] 
> > http://postgr.es/m/ca+tgmoafx2yojuhcqqol5cocei-w_ug4s2xt0etgijnpgch...@mail.gmail.com
> >
>
>
> I reviewed citext-add-extended-hash-function-v1.patch and
> hstore-add-extended-hash-function-v1.patch.
>
> I could patch and test them without trouble and could not find any issues.
>
Thanks to looking at the patch.

Regards,
Amul

Re: 64-bit hash function for hstore and citext data type

[Hironobu SUZUKI]

On 2018/09/26 11:20, amul sul wrote:

Hi all,

Commit[1] has added 64-bit hash functions for core data types and in the same
discussion thread[2] Robert Haas suggested to have the similar extended hash
function for hstore and citext data type. Attaching patch proposes the same.

Regards,
Amul

1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=81c5e46c490e2426db243eada186995da5bb0ba7
2] 
http://postgr.es/m/ca+tgmoafx2yojuhcqqol5cocei-w_ug4s2xt0etgijnpgch...@mail.gmail.com




I reviewed citext-add-extended-hash-function-v1.patch and 
hstore-add-extended-hash-function-v1.patch.


I could patch and test them without trouble and could not find any issues.

I think both patches are well.

Best regards,

A WalSnd issue related to state WALSNDSTATE_STOPPING

[Paul Guo]

Hello,

Recently I encountered a panic (assert failure) on a cassert enabled build
of Greenplum database (an MPP database based on postgres). The based
postgresql version is 9.4 stable (9.4.19). The panic is not complex. Please
see below for more details. This issue seems to be a bug in postgresql also
so I'm writing to community.

(gdb) bt

#0  0x7fb8c56321f7 in __GI_raise (sig=sig@entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7fb8c56338e8 in __GI_abort () at abort.c:90

#2  0x00ae3fd2 in ExceptionalCondition (conditionName=0xdcec75
"!(syncWalSnd)",
errorType=0xdce77a "FailedAssertion", fileName=0xdce770 "syncrep.c",
lineNumber=505) at assert.c:66
#3  0x00924dbc in SyncRepReleaseWaiters () at syncrep.c:505

#4  0x00915e5e in ProcessStandbyReplyMessage () at
walsender.c:1667
#5  0x00915bea in ProcessStandbyMessage () at walsender.c:1566

#6  0x00915ab5 in ProcessRepliesIfAny () at walsender.c:1502

#7  0x00916303 in WalSndLoop (send_data=0x916d46
) at walsender.c:1923
#8  0x0091439b in StartReplication (cmd=0x15abea8) at
walsender.c:704
#9  0x0091586e in exec_replication_command (

cmd_string=0x161fa78 "START_REPLICATION 1/8400 TIMELINE 6") at
walsender.c:1416
#10 0x00979915 in PostgresMain (argc=1, argv=0x15bafd8,
dbname=0x15baec8 "", username=0x15baea8 "gpadmin")
at postgres.c:5296


(gdb) p max_wal_senders

$1 = 1


(gdb) p WalSndCtl->walsnds[0]

$2 = {

  pid = 2765,

  state = WALSNDSTATE_STOPPING,

  sentPtr = 6509560784,

  sendKeepalive = 0 '\000',

  needreload = 0 '\000',

  write = 6509560784,

  flush = 6509560664,

  apply = 6509560664,

  caughtup_within_range = 1 '\001',

  xlogCleanUpTo = 6509560664,

  mutex = 0 '\000',

  latch = {

is_set = 1,

is_shared = 1 '\001',

owner_pid = 2765

  },

  sync_standby_priority = 1,

  synchronous = 0 '\000',

  replica_disconnected_at = 0

}


Below is related code.

  for (i = 0; i < max_wal_senders; i++)
{
/* use volatile pointer to prevent code rearrangement */
volatile WalSnd *walsnd = >walsnds[i];

if (walsnd->pid != 0 &&
walsnd->state >= WALSNDSTATE_STREAMING &&
walsnd->sync_standby_priority > 0 &&
(priority == 0 ||
 priority > walsnd->sync_standby_priority) &&
!XLogRecPtrIsInvalid(walsnd->flush))
{
priority = walsnd->sync_standby_priority;
syncWalSnd = walsnd;
}
}

/*
 * We should have found ourselves at least.
 */
Assert(syncWalSnd);

The panic reason is that since there is just one wal sender and
WalSndCtl->walsnds[0].state is WALSNDSTATE_STOPPING so syncWalSnd will be
NULL and that causes assert failure. Latest postgresql code removes the
Assert code but the related similar code logic was kept. It looks like that
we need to modify the logic similar like below (PG 9.4 STABLE) to allow
WALSNDSTATE_STOPPING which is reasonable here If I understand correctly.

if (walsnd->pid != 0 &&
-   walsnd->state == WALSNDSTATE_STREAMING &&
+   walsnd->state >= WALSNDSTATE_STREAMING &&
walsnd->sync_standby_priority > 0 &&
(priority == 0 ||
 priority > walsnd->sync_standby_priority) &&

For Latest master,  the logic is as below but it could be modified
similarly.
  /* Must be streaming */
  if (state != WALSNDSTATE_STREAMING)
  continue;

If yes this change should be on all branches that have the patch which
introduces WALSNDSTATE_STOPPING.

Thanks.

Re: Wonky rename semantics on Windows

[Thomas Munro]

On Wed, Nov 21, 2018 at 5:43 PM Thomas Munro
 wrote:
> Googling led me to some recently posted clues on SO and elsewhere
> about a flag FILE_RENAME_FLAG_POSIX_SEMANTICS which can be used with
> SetFileInformationByHandle(), that wasn't mentioned in the earlier
> -hackers threads.  I tried it on AppVeyor.  It appears to work, even
> when both source and target file name exist and both files currently
> have an open handle.  Perhaps they needed this for the Windows 10 WSL
> stuff?

Erm, looks like I posted too soon.  My 5 minute hack didn't check the
return code correctly (it's zero for failure).  Oh well.  I don't
actually plan to investigate this any further myself, being
non-Windows-enabled, but it looks like there might still be something
promising here.

--
Thomas Munro
http://www.enterprisedb.com

Wonky rename semantics on Windows

[Thomas Munro]

Hi,

I saw this failure:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2018-11-21%2000%3A51%3A32

I think there may be more than one thing going wrong there, but one
things I noticed was this:

2018-11-20 21:49:50.935 EST [9700:18] LOG:  could not rename temporary
statistics file "pg_stat_tmp/global.tmp" to "pg_stat_tmp/global.stat":
Permission denied

That probably has to do with rename() not behaving like Unix, and I
see there have been threads about this before[1].

Googling led me to some recently posted clues on SO and elsewhere
about a flag FILE_RENAME_FLAG_POSIX_SEMANTICS which can be used with
SetFileInformationByHandle(), that wasn't mentioned in the earlier
-hackers threads.  I tried it on AppVeyor.  It appears to work, even
when both source and target file name exist and both files currently
have an open handle.  Perhaps they needed this for the Windows 10 WSL
stuff?

https://github.com/macdice/hello-windows/blob/rename-test/test.c
https://ci.appveyor.com/project/macdice/hello-windows/builds/20450990

I wonder if they also have a new secret fully POSIX unlink() hidden in
there somewhere too.

[1] 
https://www.postgresql.org/message-id/flat/CAPpHfds9trA6ipezK3BsuuOSQwEmESiqj8pkOxACFJpoLpcoNw%40mail.gmail.com#9b04576b717175e9dbf03cc991977d3f

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-21 14:32:23 +1300, Thomas Munro wrote:
> On Wed, Nov 21, 2018 at 1:08 PM Andres Freund  wrote:
> > Let's see what I broke :/
>
> Per rhinoceros, contrib/sepgsql.  This patch fixes the build for me
> (on Debian with libselinux1-dev installed)

Thanks. I pushed your fix.


> though I don't know how to test that it works and make check does
> nothing useful in there.

There's a testsuite (contrib/sepgsql/test_sepgsql), but it requires
setup. I'll just let rhinoceros sort that out...

- Andres

RE: libpq debug log

[Iwata, Aya]

Hi Jim Doty san,

Thank you for review! I'm sorry my late reply...

> Initial Pass
> 
> 
> + Patch applies
> + Patch builds
> + Patch behaves as described in the thread
Thank you for your check.

> When I set a path for `PGLOGDIR` that didn't exist or was not write-able,
> the patch writes no files, and does not alert the user that no files are being
> written.
I understand. I think it means that it is necessary to confirm how the setting 
is going well. 
There is no warning method when connection string or the environment variable 
is wrong.

So I added following document:
+   If the setting of the file path by the connection string or the environment 
variable is
+   incorrect, the log file is not created in the intended location.
+   The maximum log file size you set is output to the beginning of the file, 
so you can check it.
And I added the process. Please see my v2 patch.

> Performance
> ===
> 
> I ran two permutations of make check, one with the patch applied but not
> activated, and the other with with the files being written to disk. Each
> permutation was run ten times, and the stats are below (times are in
> seconds):
> 
>   min  max  median  mean
> not logging  50.4 57.653.3  53.4
> logging  58.3 77.765.0  65.8
Thank you for your measurement. 
I'm thinking about adding a logging level so that only the necessary 
information can be printed by default. It was pointed out by Haribabu san's 
e-mail.
This minimizes the impact of logging on performance.

Regards,
Aya Iwata


v2-0001-libpq-trace-log.patch
Description: v2-0001-libpq-trace-log.patch

Re: PostgreSQL Limits and lack of documentation about them.

[David Rowley]

Thanks for looking at this.

On Thu, 15 Nov 2018 at 13:35, Tom Lane  wrote:
> * I don't like inserting this as Appendix B, because that means
> renumbering appendixes that have had their same names for a *long*
> time; for instance the release notes have been Appendix E since
> we adopted the modern division of the docs in 7.4.  So I'd put it
> below anything that's commonly-referenced.  Maybe just before
> "Acronyms"?

Seems fair. I've pushed it down to before acronyms.

> * I think I'd make the title "PostgreSQL Limitations", as it
> applies to the product not any one database.

Changed.

> * The repetition of "Maximum" in each table row seems rather
> pointless; couldn't we just drop that word?

I've changed the column header to "Upper Limit" and removed the
"Maximum" in each row.

> * Items such as "relations per database" are surely not unlimited;
> that's bounded at 4G by the number of distinct OIDs.  (In practice
> you'd get unhappy well before that, though I suppose that's true
> for many of these.)

True. I've changed this to 4,294,950,911, which is 2^32 -
FirstNormalObjectId - 1 (for InvalidOid)

> * Rows per table is also definitely finite if you are documenting
> pages per relation as finite.  But it'd be worth pointing out that
> partitioning provides a way to surmount that.

I was unsure how best to word this one. I ended up with "Limited by
the number of tuples that can fit onto 4,294,967,295 pages"

> * Many of these values are affected by BLCKSZ.  How much effort
> shall we spend on documenting that?

I've changed the comment in the maximum relation size to read
"Assuming the default BLCKSZ of 8192 bytes".

> * Max ID length is 63 bytes not characters.

Changed.

> * Don't think I'd bother with mentioning INCLUDE columns in the
> "maximum indexed columns" entry.  Also, maybe call that "maximum
> columns per index"; as phrased, it could be misunderstood to
> mean that only 32 columns can be used in all indexes put together.

I slightly disagree about INCLUDE, but I've removed it anyway. Changed
the title to "Columns per index".

> * Ordering of the table entries seems a bit random.

It ended up that way due to me having not thought of any good order.
I've changed it to try to be roughly in order of scope; database
first, then things that go in them later. Perhaps that's no good, but
it does seem better than random. I don't really think alphabetical is
useful.

> > The only other thing that sprung to my mind was the maximum tables per
> > query.  This is currently limited to 64999, not including double
> > counting partitioned tables and inheritance parents, but I kinda think
> > of we feel the need to document it, then we might as well just raise
> > the limit.
>
> Can't get excited about documenting that one ... although as things
> stand, it implies a limit on the number of partitions you can use
> that's way lower than the claimed 256M.

That is true, although that may change if we no longer reserve varnos
for pruned partitions.  More partitions could then be created, you'd
just not be able to query them all at once.  For now, I've just
removed the mention of maximum partitions as it seemed a little too
obscure to document the 64999 limit due to stepping into special varno
space.

Another thing that I was a bit unsure about is the maximum table size
limit.  I've got written that it's 32 TB, but that's not quite correct
as it's 8192 bytes less than that due to InvalidBlockNumber. Writing
"35,184,372,080,640 bytes" did not seem like an improvement.

I also altered the intro paragraph to mention practical limitations
and that the table below only mentions hard limitations.

v5 is attached.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


v5-0001-Add-documentation-section-appendix-detailing-some.patch
Description: Binary data

Re: [RFC] Removing "magic" oids

[Thomas Munro]

On Wed, Nov 21, 2018 at 1:08 PM Andres Freund  wrote:
> Let's see what I broke :/

Per rhinoceros, contrib/sepgsql.  This patch fixes the build for me
(on Debian with libselinux1-dev installed), though I don't know how to
test that it works and make check does nothing useful in there.

-- 
Thomas Munro
http://www.enterprisedb.com


fix-sepgsql.patch
Description: Binary data

Re: Add extension options to control TAP and isolation tests

[Michael Paquier]

On Tue, Nov 20, 2018 at 07:30:39PM +0300, Nikolay Shaplov wrote:
> Is it ok, if I join the reviewing? I like test, especially TAP one, you know 
> ;-)
>
> Since you are much more experienced in postgres then me, I'd try to
> understand how does the patch work, try to use if for writing more TAP
> test, and will report problems and thoughts I came across while doing that.

Thanks for bumping in the field.

> For me name "output_iso" means nothing. iso is something about CD/DVD or 
> about 
> standards. I would not guess that iso stands for isolation if I did not know 
> it already. isolation_output is more sensible: I have heard that there are 
> some isolation tests, this must be something about it. May be it would be 
> better to change it to isolation_output everywhere instead of changing to 
> output_iso

That's just a default configuration used by the isolation tester.
That's not much bothering with in my opinion for this patch, as the
patch is here to make sure that the default configuration gets used
where it had better be used.  Changing this default value would be of
course doable technically, but that's around for years to changing it
does not seem like good idea.

> I tried to find definition in documentation what does "isolation test" 
> exactly 
> means, but did not find. There is some general words about TAP tests in main 
> postgres documentation 
> https://www.postgresql.org/docs/11/regress-tap.html,
> but I would not understand anything from it if I did not already know how it 
> works.

Those are mentioned here as part of the additional test suites:
https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5

> In current extend-pgxs documentation there is some explanation about 
> regression test, it sensible enough. Since TAP and isolation tests are 
> introduced now, there should be same short explanation for both of
> them.

I see your point here, and it is true that documentation ought to be
better.  So I have written out a new set of paragraphs which explain the
whereabouts of the new variables a bit more in depth in the section of
extend-pgxs.

> And also it would be good to add links from extend-pgxs to regress-tap and 
> regress saying that for more info about these tests one can look at postgres 
> doc, because they work in a similar way.

I have added a reference to regress-tap in one of the new paragraphs.
Linking the existing stuff to point to "regress" is a separate issue
though, and while pointing to the TAP section is adapted as its
guidelines are rather general, I am not sure which one would make the
most sense though.
--
Michael
diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
index 13bd397b70..da9553a2d0 100644
--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -8,6 +8,7 @@ DATA = bloom--1.0.sql
 PGFILEDESC = "bloom access method - signature file based index"
 
 REGRESS = bloom
+TAP_TESTS = 1
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
@@ -19,6 +20,3 @@ top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
-
-wal-check: temp-install
-	$(prove_check)
diff --git a/contrib/oid2name/Makefile b/contrib/oid2name/Makefile
index 908e078714..361a80a7a1 100644
--- a/contrib/oid2name/Makefile
+++ b/contrib/oid2name/Makefile
@@ -6,11 +6,11 @@ PGAPPICON = win32
 PROGRAM = oid2name
 OBJS	= oid2name.o $(WIN32RES)
 
+TAP_TESTS = 1
+
 PG_CPPFLAGS = -I$(libpq_srcdir)
 PG_LIBS_INTERNAL = $(libpq_pgport)
 
-EXTRA_CLEAN = tmp_check
-
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -21,9 +21,3 @@ top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
-
-check:
-	$(prove_check)
-
-installcheck:
-	$(prove_installcheck)
diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index afcab930f7..8cd83a763f 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -3,9 +3,20 @@
 MODULES = test_decoding
 PGFILEDESC = "test_decoding - example of a logical decoding output plugin"
 
-# Note: because we don't tell the Makefile there are any regression tests,
-# we have to clean those result files explicitly
-EXTRA_CLEAN = $(pg_regress_clean_files)
+EXTRA_INSTALL=contrib/test_decoding
+
+REGRESS = ddl xact rewrite toast permissions decoding_in_xact \
+	decoding_into_rel binary prepared replorigin time messages \
+	spill slot truncate
+ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
+	oldest_xmin snapshot_transfer
+
+REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
+ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
+
+# Disabled because these tests require "wal_level=logical", which
+# typical installcheck users do not have (e.g. buildfarm clients).
+NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
@@ -18,52 +29,8 @@ include $(top_builddir)/src/Makefile.global
 include 

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-20 01:52:27 -0800, Andres Freund wrote:
> I'm pretty happy with the new state. Unless somebody announces they want
> to do a review soon-ish, I'm planning to commit this soon. It's a
> painful set to keep up2date, and it's blocking a few other patches.  I'm
> sure we'll find some things to adapt around the margins, but imo the
> patch as a whole looks pretty reasonable.
> 
> Missing:
> - nice and long commit message
> - another detailed line-by-line read of the patch (last round took like
>   3h :()
> 
> 341 files changed, 2263 insertions(+), 4249 deletions(-)

I've now pushed this. There's one bugfix (forgot to unset replaces for
the oid column in heap_modify_tuple), and one mostly cosmetic change:
Previously the .bki insert lines had the oid duplicated between OID =
and the normal column, now it's just the latter.

Let's see what I broke :/

Regards,

Andres

Re: CF app feature request

[Michael Paquier]

On Tue, Nov 20, 2018 at 03:30:38PM -0300, Alvaro Herrera wrote:
> On 2018-Nov-20, Tom Lane wrote:
> Certainly not higher than having the dropdown for entry author/reviewer
> be sorted alphabetically ... *wink* *wink*

More *wink* *wink*
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] XLogReadRecord returns pointer to currently read page

[Michael Paquier]

On Tue, Nov 20, 2018 at 06:37:36AM +0500, Andrey Lepikhov wrote:
> On 20.11.2018 6:30, Michael Paquier wrote:
>> Yeah, this one is not entirely true now.  What about the following
>> instead:
>> -   /* Buffer for current ReadRecord result (expandable) */
>> +   /*
>> +* Buffer for current ReadRecord result (expandable), used when a record
>> +* crosses a page boundary.
>> +*/
> 
> I agree

Okay, I have committed this version, after checking that there were no
other spots.
--
Michael


signature.asc
Description: PGP signature

Re: A small tweak to some comments for PartitionKeyData

[David Rowley]

On Fri, 16 Nov 2018 at 11:23, Peter Eisentraut
 wrote:
> committed

Thank you.


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: heap_sync seems rather oblivious to partitioned tables (wal_level=minimal)

[David Rowley]

(Returning from leave)

On Tue, 20 Nov 2018 at 03:19, Alvaro Herrera  wrote:
> Pushed now, thanks.

Many thanks for pushing.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: heap_sync seems rather oblivious to partitioned tables (wal_level=minimal)

[David Rowley]

On Tue, 20 Nov 2018 at 01:47, Tomas Vondra  wrote:
> The patch seems fine to me.

Thanks for looking at it.

> It's a bit unfortunate that we simply disable the optimization on
> partitioned tables instead of fixing it somehow, but I guess it's better
> than nothing and no one has a very good idea how to make it work.

Yeah, I agree.  I added various comments in the patch to explain why
we're not going to the trouble of allowing it. I had hoped that would
do for now.  Lately, there's quite a bit of effort going into reducing
various overheads to having a partitioned table with high numbers of
partitions. A patch to speed up INSERT was just committed, for
example.  Perhaps COPY is less critical to make faster for inserting
small numbers of rows, but I'd not rule out someone complaining
sometime about the overhead of checking all partitions are compatible
with the freeze optimisation before doing the COPY, although, perhaps
we can just tell those people not to use the FREEZE option.

Anyway, I was mostly interested in fixing the bug.  Making COPY FREEZE
work with partitioned tables sounds like a new feature.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: pgbench - doCustom cleanup

[Alvaro Herrera]

On 2018-Nov-20, Fabien COELHO wrote:

> Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
> transitions to happen in doCustom's switch (st->state) and nowhere else,
> which is defeated by creating the separate function.
> 
> Although it improves readability at one level, it does not help figuring out
> what happens to states, which is my primary concern: The idea is that
> reading doCustom is enough to build and check the automaton, which I had to
> do repeatedly while reviewing Marina's patches.

I adopted your patch, then while going over it I decided to go with my
separation proposal anyway, and re-did it.

Looking at the state of the code before this patch, I totally understand
that you want to get away from the current state of affairs.  However, I
don't think my proposal makes matters worse; actually it makes them
better.  Please give the code a honest look and think whether the
separation of machine state advances is really worse with my proposal.

I also renamed some things.  doCustom() was a pretty bad name, as was
CSTATE_START_THROTTLE.

> Also the added doCustom comment, which announces this property becomes false
> under the refactoring function:
> 
>   All state changes are performed within this function called by 
> threadRun.
> 
> So I would suggest not to create this function.

I agree the state advances in threadRun were real messy.

Thanks for getting rid of the goto.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 73d3de0677..710130b022 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -294,24 +294,32 @@ typedef enum
 {
 	/*
 	 * The client must first choose a script to execute.  Once chosen, it can
-	 * either be throttled (state CSTATE_START_THROTTLE under --rate) or start
-	 * right away (state CSTATE_START_TX).
+	 * either be throttled (state CSTATE_PREPARE_THROTTLE under --rate), start
+	 * right away (state CSTATE_START_TX) or not start at all if the timer was
+	 * exceeded (state CSTATE_FINISHED).
 	 */
 	CSTATE_CHOOSE_SCRIPT,
 
 	/*
-	 * In CSTATE_START_THROTTLE state, we calculate when to begin the next
+	 * In CSTATE_PREPARE_THROTTLE state, we calculate when to begin the next
 	 * transaction, and advance to CSTATE_THROTTLE.  CSTATE_THROTTLE state
-	 * sleeps until that moment.  (If throttling is not enabled, doCustom()
-	 * falls directly through from CSTATE_START_THROTTLE to CSTATE_START_TX.)
+	 * sleeps until that moment.
+	 *
+	 * It may also detect that the next transaction would start beyond the end
+	 * of run, and switch to CSTATE_FINISHED.
 	 */
-	CSTATE_START_THROTTLE,
+	CSTATE_PREPARE_THROTTLE,
 	CSTATE_THROTTLE,
 
 	/*
 	 * CSTATE_START_TX performs start-of-transaction processing.  Establishes
-	 * a new connection for the transaction, in --connect mode, and records
-	 * the transaction start time.
+	 * a new connection for the transaction in --connect mode, records the
+	 * transaction start time, and proceed to the first command.
+	 *
+	 * Note: once a script is started, it will either error or run till its
+	 * end, where it may be interrupted. It is not interrupted while running,
+	 * so pgbench --time is to be understood as tx are allowed to start in
+	 * that time, and will finish when their work is completed.
 	 */
 	CSTATE_START_TX,
 
@@ -324,9 +332,6 @@ typedef enum
 	 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 	 * meta-commands are executed immediately.
 	 *
-	 * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
-	 * quickly skip commands that do not need any evaluation.
-	 *
 	 * CSTATE_WAIT_RESULT waits until we get a result set back from the server
 	 * for the current command.
 	 *
@@ -334,19 +339,25 @@ typedef enum
 	 *
 	 * CSTATE_END_COMMAND records the end-of-command timestamp, increments the
 	 * command counter, and loops back to CSTATE_START_COMMAND state.
+	 *
+	 * CSTATE_SKIP_COMMAND is used by conditional branches which are not
+	 * executed. It quickly skip commands that do not need any evaluation.
+	 * This state can move forward several commands, till there is something
+	 * to do or the end of the script.
 	 */
 	CSTATE_START_COMMAND,
-	CSTATE_SKIP_COMMAND,
 	CSTATE_WAIT_RESULT,
 	CSTATE_SLEEP,
 	CSTATE_END_COMMAND,
+	CSTATE_SKIP_COMMAND,
 
 	/*
-	 * CSTATE_END_TX performs end-of-transaction processing.  Calculates
-	 * latency, and logs the transaction.  In --connect mode, closes the
-	 * current connection.  Chooses the next script to execute and starts over
-	 * in CSTATE_START_THROTTLE state, or enters CSTATE_FINISHED if we have no
-	 * more work to do.
+	 * CSTATE_END_TX performs end-of-transaction processing.  It calculates
+	 * latency, and logs the transaction.  In --connect mode, it closes the
+	 * current connection.
+	 *
+	 * Then either starts over in CSTATE_CHOOSE_SCRIPT, 

Re: pgbench - doCustom cleanup

[Alvaro Herrera]

On 2018-Nov-20, Fabien COELHO wrote:

> 
> > I didn't quite understand this hunk.  Why does it remove the
> > is_latencies conditional?  (The preceding comment shown here should be
> > updated obviously if this change is correct, but I'm not sure it is.)
> 
> Pgbench runs benches a collects performance data about it.
> 
> I simplified the code to always collect data, without trying to be clever
> about cases where these data may not be useful so some collection can be
> skipped.
> 
> Here the test avoids recording the statement start time, mostly a simple
> assignment and then later another test avoids recording the stats in the
> same case, which are mostly a few adds.
> 
> ISTM that this is over optimization and unlikely to be have any measurable
> effects compared to the other tasks performed when executing commands, so a
> simpler code is better.

I don't think we're quite ready to buy this argument just yet.
See https://www.postgresql.org/message-id/flat/31856.1400021...@sss.pgh.pa.us

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Thomas Munro]

On Wed, Nov 21, 2018 at 9:07 AM Andres Freund  wrote:
> On 2018-11-21 09:00:58 +1300, Thomas Munro wrote:
> > On Wed, Nov 21, 2018 at 4:37 AM REIX, Tony  wrote:
> > > YES ! Reading this file, your suggestion should work ! Thx !
> > >
> > > I've rebuilt and run the basic tests. We'll relaunch our tests asap.
> >
> > I would be surprised if that makes a difference:
> > anonymous-mmap-then-fork and SysV shm are just two different ways to
> > exchange mappings between processes, but I'd expect the virtual memory
> > object itself to be basically the same, in terms of constraints that
> > might affect page size at least.
>
> I don't think that's true on many systems, FWIW. On linux there's
> certainly different behaviour, and e.g. the way to get hugepages for
> anon-mmap and SysV shmem aren't the same.

Right, when asking for them explicitly the API is different (SHM_HUGE
flag to shmget(), MAP_HUGETLB flag to mmap()).  Actually I was
expecting AIX to be more like FreeBSD and Solaris, where you don't do
that, the OS just decides what page size to give you, but after some
quality time with google I now see that it's more like Linux in the
SysV case... there is an explicit flag:

https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.performance/large_pages_shared_mem_segs.htm

You also need some special privileges:

https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.performance/large_page_ovw.htm

As for the main shared buffers area using anon-mmap, I wonder if it
would automagically use large pages if you have the privileges and set
the LDR_CNTRL environment variable (or the equivalent XCOFF header for
the binary):

https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/com.ibm.aix.performance/set_env_variable_lpages.htm

> [1] strongly suggests that
> that's not the case on FreeBSD either (with sysv shmem being
> better). I'd attached a patch to implement a GUC to allow users to
> choose the shmem implementation back then [2].

Surprising.  I'd like to know if that's still true.  SysV shm is not
nice, and if there is anything accidentally better about its
performance, I'd love to know what.  That report (slightly) predates
this work (maybe causally linked), which fixed various VM scale
problems hit by PostgreSQL:
http://www.kib.kiev.ua/kib/pgsql_perf_v2.0.pdf

-- 
Thomas Munro
http://www.enterprisedb.com

Re: pg_stat_ssl additions

[Peter Eisentraut]

On 20/11/2018 09:31, Kyotaro HORIGUCHI wrote:
> Comparing the two codes in pgstatfuncs.c and sslinfo.c, It seems
> that ssl_client_serial() can be largely simplified using
> be_tls_get_peer_serial(). X509_NAME_to_text() can be simplified
> using X509_NAME_to_cstring(). But this would be another patch.
> 
> By the way, though it is not by this patch, X509_NAME_to_cstring
> doesn't handle NID_undef from OBJ_obj2nid() and NULL from
> OBJ_nid2ln(). The latter case feeds NULL to BIO_printf() . I'm
> not sure it can actually happen.

Yes, that seems problematic, at least in theory.

>> - Changes pg_stat_ssl.clientdn to be null if there is no client
>> certificate (as documented, but not implemented). (bug fix)
> 
> This reveals DN, serial and issuer DN of the cert to
> non-superusres. pg_stat_activity hides at least
> client_addr. Aren't they needed to be hidden? (This will change
> the existing behavior.)

Yes.  Arguably, nothing in this view other than your own session should
be seen by normal users.  Thoughts from others?

>> - Adds new fields to pg_stat_ssl: issuerdn and clientserial.  These
>> allow uniquely identifying the client certificate.  AFAICT, these are
>> the most interesting pieces of information provided by sslinfo but not
>> in pg_stat_ssl.  (I don't like the underscore-free naming of these
>> fields, but it matches the existing "clientdn".)
> 
> clientdn, clientserial, issuerdn are the fields about client
> certificates. Only the last one omits prefixing "client". But
> "clientissuerdn" seems somewhat rotten... Counldn't we rename
> clientdn to client_dn?

I'd prefer renaming as well, but some people might not like that.

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

Re: settings to control SSL/TLS protocol version

[Peter Eisentraut]

On 04/11/2018 04:24, Steve Singer wrote:
> The feature seems useful a lot of application servers are implementing 
> minimal TLS protocol versions.
> I don't see a way to restrict libpq to only connect with certain protocol 
> versions.  Maybe that is a separate patch but it would make this feature 
> harder to test in the future.

Client-side support could be separate patch, yes.  It seems less important.

> I am updating the patch status to ready for committer.
> 
> The new status of this patch is: Ready for Committer

Committed with the change 'any' -> '' that was discussed elsewhere in
the thread.  Thanks.

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

Re: BUG #15160: planner overestimates number of rows in join when there are more than 200 rows coming from CTE

[Melanie Plageman]

Given that you have addressed all of my feedback and that it's a pretty
low-risk change, I will change the status to "ready for committer".

There are a couple of minor follow-up clarifications inline that relate
mostly
to the questions that I asked in previous emails.

I did have one other question:
Has there been discussion in the past about adding a planner test extension
similar to those in src/test/modules for cardinality estimation? I am
imagining
something that is a "soft" check that either the rows estimation that comes
out
of calc_joinrel_size_estimate is within an expected range (given differing
estimates across machines) or that the selectivity estimate that comes out
of
eqjoinsel is within an expected range. The former seems relatively easy to
do
in a manner similar to the test_predtest extension and the latter seems
like it
could be done even more trivially.

On Sat, Nov 17, 2018 at 12:22 PM Tom Lane  wrote:

> (Here, both "outer rel's size" and "inner rel's size" mean the size after
> earlier filtering steps.)  So that's why we only clamp nd2 and only do so
> in eqjoinsel_semi: in the other three cases, we'd be double-counting the
> selectivity of earlier filters if we did that.
>
> I just want to make sure I am understanding what the comment is saying: So,
after we calculate the selectivity for inner join, when we return from
calc_joinrel_size_estimate we do this math:

nrows = outer_rows * inner_rows * fkselec * jselec;

and in that equation, the outer and inner rows have been adjusted to account
for any restrictions on the tables, so we don't clamp the ndvs for inner
join
in eqjoinsel_inner. However, for semi-join, that calculation is

nrows = outer_rows * fkselec * jselec;

Which means that we have to adjust the rows of the inner side before we get
here?


> So basically the inconsistency here comes from the fact that we define
> the meaning of join selectivity differently for inner and semi joins.
> I've occasionally wondered if that was a bad choice and we should just
> say that selectivity should always be calculated so that the join size
> is outer size times inner size times selectivity.  But that would
> certainly not make for any less need for the selectivity estimator to
> do things differently for inner and semi joins, so I am not seeing much
> upside to changing it.
>

I see what you are saying. I got tangled up in this part of the code, so I
am
inclined to say that it could stand to be more clear. Selectivity is a
ratio,
and, even if you calculate the two sides of the ratio differently, that
doesn't
mean the definition of the ratio should be different.

Also, I wanted to address a question you asked in an earlier email:
You wrote:
> Hm.  Maybe the "Psemi" and "Pinner" notation is not helpful ... would
> "Ssemi" and "Sinner" be better?

I think Ssemi and Sinner might be more clear--mostly because we haven't used
P/predicate here or in most of the other selectivity estimation comments
that I
read. Also, in some cases when we have super limited information and make a
guess, the selectivity feels pretty detached from the join predicate.

Thanks!

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Andres Freund]

Hi,

On 2018-11-21 09:00:58 +1300, Thomas Munro wrote:
> On Wed, Nov 21, 2018 at 4:37 AM REIX, Tony  wrote:
> > YES ! Reading this file, your suggestion should work ! Thx !
> >
> > I've rebuilt and run the basic tests. We'll relaunch our tests asap.
> 
> I would be surprised if that makes a difference:
> anonymous-mmap-then-fork and SysV shm are just two different ways to
> exchange mappings between processes, but I'd expect the virtual memory
> object itself to be basically the same, in terms of constraints that
> might affect page size at least.

I don't think that's true on many systems, FWIW. On linux there's
certainly different behaviour, and e.g. the way to get hugepages for
anon-mmap and SysV shmem aren't the same. [1] strongly suggests that
that's not the case on FreeBSD either (with sysv shmem being
better). I'd attached a patch to implement a GUC to allow users to
choose the shmem implementation back then [2].

[1] 
http://archives.postgresql.org/message-id/2AE143D2-87D3-4AD1-AC78-CE2258230C05%40FreeBSD.org
[2] 
http://archives.postgresql.org/message-id/20140422121921.GD4449%40awork2.anarazel.de

Greetings,

Andres Freund

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Thomas Munro]

On Wed, Nov 21, 2018 at 4:37 AM REIX, Tony  wrote:
> YES ! Reading this file, your suggestion should work ! Thx !
>
> I've rebuilt and run the basic tests. We'll relaunch our tests asap.

I would be surprised if that makes a difference:
anonymous-mmap-then-fork and SysV shm are just two different ways to
exchange mappings between processes, but I'd expect the virtual memory
object itself to be basically the same, in terms of constraints that
might affect page size at least.

If you were talking about mmap backed by a file (which is what you get
for temporary parallel query segments if you tell it to use
dynamic_shared_memory_type = mmap), that might be a different matter,
because then the block size of the file system backing it might come
into the picture and limit the kernel's options.  For example, that is
why (with default settings) Parallel Hash can't use large pages on
Linux (because Linux's POSIX shm_open() really just opens files on
/dev/shm, which has a 4k block size), but can use them on FreeBSD
(because its shm_open() isn't bound to page sizes, it can and
sometimes decides to use large pages).

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-20 21:49:27 +0700, John Naylor wrote:
> On 11/20/18, Andres Freund  wrote:
> > I'm
> > sure we'll find some things to adapt around the margins, but imo the
> > patch as a whole looks pretty reasonable.
> 
> bki.sgml is now a bit out of date. I've attached a draft fix.

Thanks!

Greetings,

Andres Freund

Re: [RFC] Removing "magic" oids

[Andres Freund]

Hi,

On 2018-11-20 11:25:22 -0500, Stephen Frost wrote:
> Greetings,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Sun, Oct 14, 2018 at 6:35 PM Tom Lane  wrote:
> > > Andres Freund  writes:
> > > > Does anybody have engineering / architecture level comments about this
> > > > proposal?
> > >
> > > FWIW, I'm -1 on making OIDs be not-magic for SELECT purposes.  Yeah, it's
> > > a wart we wouldn't have if we designed the system today, but the wart is
> > > thirty years old.  I think changing that will break so many catalog
> > > queries that we'll have the villagers on the doorstep.  Most of the other
> > > things you're suggesting here could be done easily without making that
> > > change.
> > >
> > > Possibly we could make them not-magic from the storage standpoint (ie
> > > they're regular columns) but have a pg_attribute flag that says not
> > > to include them in "SELECT *" expansion.
> > 
> > I think such a flag would be a good idea; it seems to have other uses.
> > As Andres also noted, Kevin was quite interested in having a
> > hidden-by-default COUNT column to assist with materialized view
> > maintenance, and presumably this could also be used for that purpose.
> 
> Yeah, I like the idea of having this column too, it'd also be useful for
> people who are trying to use column-level privileges.

FWIW, leaving grammar bikeshedding aside, I don't think this is
particularly hard.  There certainly are a few corner cases I've not
touched here, but the attached implements the basic features for hidden
columns.


postgres[14805][1]=# CREATE TABLE blarg(id serial, data text, annotation text);
postgres[14805][1]=# INSERT INTO blarg (data, annotation) VALUES ('42', 'this 
is THE question');

postgres[14805][1]=# SELECT * FROM blarg;
┌┬──┬──┐
│ id │ data │  annotation  │
├┼──┼──┤
│  1 │ 42   │ this is THE question │
└┴──┴──┘
(1 row)

postgres[14805][1]=# SELECT s.* FROM (SELECT * FROM blarg) s;
┌┬──┬──┐
│ id │ data │  annotation  │
├┼──┼──┤
│  1 │ 42   │ this is THE question │
└┴──┴──┘
(1 row)

postgres[14805][1]=# SELECT s.* FROM (SELECT blarg FROM blarg) s;
┌───┐
│ blarg │
├───┤
│ (1,42,"this is THE question") │
└───┘
(1 row)


postgres[14805][1]=# SELECT s.annotation FROM (SELECT * FROM blarg) s;
┌──┐
│  annotation  │
├──┤
│ this is THE question │
└──┘
(1 row)

postgres[14805][1]=# SELECT (s.blarg).annotation FROM (SELECT blarg FROM blarg) 
s;
┌──┐
│  annotation  │
├──┤
│ this is THE question │
└──┘
(1 row)


-- update column to be hidden
postgres[14805][1]=# UPDATE pg_attribute SET attishidden = true WHERE attrelid 
= 'blarg'::regclass AND attname = 'annotation';

postgres[14805][1]=# SELECT * FROM blarg;
┌┬──┐
│ id │ data │
├┼──┤
│  1 │ 42   │
└┴──┘
(1 row)

postgres[14805][1]=# SELECT s.* FROM (SELECT * FROM blarg) s;
┌┬──┐
│ id │ data │
├┼──┤
│  1 │ 42   │
└┴──┘
(1 row)

postgres[14805][1]=# SELECT s.blarg FROM (SELECT blarg FROM blarg) s;
┌───┐
│ blarg │
├───┤
│ (1,42,"this is THE question") │
└───┘
(1 row)

postgres[14805][1]=# SELECT s.annotation FROM (SELECT * FROM blarg) s;
ERROR:  42703: column s.annotation does not exist
LINE 1: SELECT s.annotation FROM (SELECT * FROM blarg) s;
   ^
LOCATION:  errorMissingColumn, parse_relation.c:3319

postgres[14805][1]=# SELECT (s.blarg).annotation FROM (SELECT blarg FROM blarg) 
s;
┌──┐
│  annotation  │
├──┤
│ this is THE question │
└──┘
(1 row)

postgres[14805][1]=# SELECT (s.blarg).* FROM (SELECT blarg FROM blarg) s;
┌┬──┐
│ id │ data │
├┼──┤
│  1 │ 42   │
└┴──┘
(1 row)


It's debatable if a wholerow-var select (without *, i.e the s.blarg case
above)) ought to include the hidden column, but to me that intuitively
makes sense. Otherwise you couldn't select it after a cast and such.

Greetings,

Andres Freund
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 5354a04639b..78934c11de1 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -461,6 +461,8 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
 			return false;
 		if (attr1->attislocal != attr2->attislocal)
 			return false;
+		if (attr1->attishidden != attr2->attishidden)
+			return false;
 		if (attr1->attinhcount != attr2->attinhcount)
 			return false;
 		if (attr1->attcollation != attr2->attcollation)
@@ -641,6 +643,7 @@ 

Re: Refactoring the checkpointer's fsync request queue

[Robert Haas]

On Fri, Nov 16, 2018 at 5:38 PM Thomas Munro
 wrote:
> Or to put it another way, you can't be given a lower sequence number
> than another process that has already written, because that other
> process must have been given a sequence number before it wrote.

OK, that makes sense.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: CF app feature request

[Alvaro Herrera]

On 2018-Nov-20, Tom Lane wrote:

> I think there is a use-case for "Withdrawn", it's more polite than
> "Rejected" ;-).  But it's not a very high-priority request.

Certainly not higher than having the dropdown for entry author/reviewer
be sorted alphabetically ... *wink* *wink*

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: CF app feature request

[Tom Lane]

Magnus Hagander  writes:
> I'm trying to figure out where this thread left off :) My understanding of
> the consensus is we don't actually want/need a change in the app, but are
> instead OK with the admin just handling it a somewhat ugly way in the few
> cases where it's necessary?

The original case (just a mistakenly duplicated entry) seems OK to solve
with a quick DELETE on the underlying table.

> Or is the consensus to add a "Withdrawn" status, just to solve a slightly
> different problem from the one that started this thread?

I think there is a use-case for "Withdrawn", it's more polite than
"Rejected" ;-).  But it's not a very high-priority request.

regards, tom lane

Re: Connection slots reserved for replication

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Fri, Nov 9, 2018 at 2:02 AM Masahiko Sawada 
> wrote:
> > On the other hand, If we always reserve max_wal_senders slots
> > available slots for normal backend will get decreased in the next
> > release, which require for users to re-confiugre the max_connection.
> > But I felt this behavior seems more natural than the current one, so I
> > think the re-configuration can be acceptable for users.
>
> Maybe what we should do instead is not consider max_wal_senders a part of
> the total number of connections, and instead size the things that needs to
> be sized by them by max_connections + max_wal_senders. That seems more
> logical given how the parameters are named as well.

I tend to agree with having max_connections + max_wal_senders.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: CF app feature request

[Magnus Hagander]

On Sun, Nov 4, 2018 at 1:28 AM Michael Paquier  wrote:

> On Fri, Nov 02, 2018 at 09:15:36PM +0100, Dmitry Dolgov wrote:
> > Just to make sure, if a duplicated entry will be removed, the patch
> itself
> > will stay or not? I'm asking, because both entries have the same patch
> > referenced, and the admin form says that one of the related items, that
> > would be removed is the patch item.
>
> If you remove only one entry, its references will be removed but the
> second one will remain.  If you want me to proceed, I can do so.  I have
> done that in the past, and it is not the first time someone registers a
> duplicated entry in the CF app.
>

I'm trying to figure out where this thread left off :) My understanding of
the consensus is we don't actually want/need a change in the app, but are
instead OK with the admin just handling it a somewhat ugly way in the few
cases where it's necessary?

Or is the consensus to add a "Withdrawn" status, just to solve a slightly
different problem from the one that started this thread?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: Connection slots reserved for replication

[Magnus Hagander]

On Fri, Nov 9, 2018 at 2:02 AM Masahiko Sawada 
wrote:

> On Thu, Nov 8, 2018 at 9:30 PM Kyotaro HORIGUCHI
>  wrote:
> >
> > Hello.
> >
> > At Wed, 7 Nov 2018 19:31:00 +0900, Masahiko Sawada <
> sawada.m...@gmail.com> wrote in  evkgm-56...@mail.gmail.com>
> > > On Tue, Nov 6, 2018 at 9:16 PM Kyotaro HORIGUCHI
> > >  wrote:
> > > > InitializeMaxBackends()
> > > > MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
> > > > -   max_worker_processes;
> > > > +   max_worker_processes +
> replication_reserved_connections;
> > > >
> > > > This means walsender doesn't comsume a connection, which is
> > > > different from the current behavior. We should reserve a part of
> > > > MaxConnections for walsenders.  (in PostmasterMain,
> > > > max_wal_senders is counted as a part of MaxConnections)
> > >
> > > Yes. We can force replication_reserved_connections <= max_wal_senders
> > > and then reserved connections for replication should be a part of
> > > MaxConnections.
> > >
> > > >
> > > > +   if (am_walsender && replication_reserved_connections <
> max_wal_senders
> > > > +   && *procgloballist == NULL)
> > > > +   procgloballist = >freeProcs;
> > > >
> > > > Currently exccesive number of walsenders are rejected in
> > > > InitWalSenderSlot and emit the following error.
> > > >
> > > > >ereport(FATAL,
> > > > >(errcode(ERRCODE_TOO_MANY_CONNECTIONS),
> > > > > errmsg("number of requested standby connections "
> > > > >"exceeds max_wal_senders (currently %d)",
> > > > >max_wal_senders)));
> > > >
> > > > With this patch, if max_wal_senders =
> > > > replication_reserved_connections = 3 and the fourth walreceiver
> > > > comes, we will get "FATAL: sorry, too many clients already"
> > > > instead. It should be fixed.
> > > >
> > > > When r_r_conn = 2 and max_wal_senders = 3 and the three
> > > > walsenders are active, in an exreme case where a new replication
> > > > connection comes at the same time another is exiting, we could
> > > > end up using two normal slots despite that one slot is vacant in
> > > > reserved slots.
> > >
> > > Doesn't the max_wal_senders prevent the case?
> >
> > Currently the variable doesn't work as so. We once accept the
> > connection request and searches for a vacant slot in
> > InitWalSenderSlot and reject the connection if it found that no
> > room is available. Even with this patch, we don't count the
> > accurate number of active walsenders (for performance reason). If
> > reserved slot are filled, there's no way other than to accept the
> > connection using non-reserved slot if r_r_conn <
> > max_wal_senders. If one of active walsenders went away since we
> > allocated non-reserved connection slot until InitWalSenderSlot
> > starts searching sendnds[] array. Finally the new walsender on
> > the unreserved slot is activated, and one reserved slot is left
> > empty. So this is "an extreme case". We could ignore the case.
> >
> > I'm doubt that we should allow the setting where r_r_conn <
> > max_wal_senders, or even r_r_conn != max_wal_senders. We don't
> > have a problem like this if we don't allow the cases.
> >
> > > Wal senders can get connection if we have free procs more than
> > > (MaxConnections - reserved for superusers). So I think for normal
> > > users the connection must be refused if (MaxConnections - (reserved
> > > for superuser and replication) > # of freeprocs) and for wal senders
> > > the connection also must be refused if (MaxConnections - (reserved for
> > > superuser) > # of freeprocs). I'm not sure we need such trick in
> > > InitWalSenderSlot().
> >
> > (For clarity, I don't mean my previous patch is good solution.)
> >
> > It works as far as we accept that some reserved slots can be left
> > unused despite of some walsenders are using normal slots. (Just
> > exiting a walsender using reserved slot causes this but it is
> > usually occupied by walsenders comes later)
> >
> > Another idea is we acquire a walsnd[] slot before getting a
> > connection slot..
>
> After more thought, I'm inclined to agree to reserve max_wal_senders
> slots and not to have replication_reserved_connections parameter.
>
> For superuser_reserved_connection, actually it works so that we
> certainly reserve slots for superuser in case where slots are almost
> full regardless of who is using other slots incluing superusers
> themselves. But replication connections requires different behaviour
> as it has the another limit (max_wal_senders). If we have
> replication_reserved_connections < max_wal_senders, it could end up
> with the same issue as what originally reported on this thread.
> Therefore many users would set replication_reserved_connections =
> max_wal_senders.
>
> On the other hand, If we always reserve max_wal_senders slots
> available slots for normal backend will get decreased in the next
> release, which require for users to re-confiugre the max_connection.
> 

Re: mysql_fdw crash

[Mithun Cy]

On Tue, Nov 20, 2018 at 7:59 PM Tomas Vondra
 wrote:
>
> On 11/20/18 3:06 PM, 066ce...@free.fr wrote:
> > Hi,
> >
> >> When gdb will be active, then use command c, and then run query in 
> >> session. gdb should to catch segfault.
> >
> > Thank you very much. It's been helpfull.
> >
> > BTW behaviour is strange. When I'm executing following, I do have always a 
> > SEGV :
> >
> > psql (11.1)
> > Type "help" for help.
> >
> > herve=# CREATE OR REPLACE FUNCTION public.test_bug2(text,integer,timestamp 
> > with time zone)
> > herve-#  RETURNS integer
> > herve-#
> > herve-# AS '
> > herve'#
> > herve'# select coalesce(max(id),1) from sact_v1.autocalls where  
> > label=$1 and machine_id=$2 and created_date=$3;
> > herve'# '
> > herve-#  LANGUAGE sql;
> > CREATE FUNCTION
> > herve=# select test_bug2('BSM_CRITICAL_SYSLOG',18843,now());
> >
> > The GDB session :
> >
> > Continuing.
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > prepare_query_params (param_types=0x1c86ac8, param_values=0x1c86ac0, 
> > param_exprs=0x1c86ab8, param_flinfo=0x1c86ab0, numParams=3, 
> > fdw_exprs=0x1c6b5b8, node=0x1c792d8) at mysql_fdw.c:2139
> > 2139*param_types[i] = exprType(param_expr);
> > (gdb) bt
>
> So which part of that expression triggers the segfault? Try printing the
> different parts, i.e.
>
>  p i
>  p param_types[i]
>
> It might be helpful to also install the debug package, which would give
> us more readable backtraces.
>
> BTW, considering the failure is in mysql_fdw.c, this very much seems
> like a bug in mysql_fdw - have you tried reporting it through the
> project github repository?
>
>  https://github.com/EnterpriseDB/mysql_fdw/issues
>
> That's probably more likely to help, and even if we find a bug here we
> can't really commit that (perhaps some of the mysql_fdw authors are
> watching this list, but I'm not sure about that).

Thanks for reporting,

 Oid **param_types)
{
int i;
ListCell   *lc;

Assert(numParams > 0);

/* Prepare for output conversion of parameters used in remote query. */
*param_flinfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo) * numParams);

*param_types = (Oid *) palloc0(sizeof(Oid) * numParams);

i = 0;
foreach(lc, fdw_exprs)
{
Node   *param_expr = (Node *) lfirst(lc);
Oid typefnoid;
boolisvarlena;

*param_types[i] = exprType(param_expr);

Seems some basic mistake I think it should as below
(*param_types)[i] = exprType(param_expr);

After this it works
postgres=#  select test_bug2('BSM_CRITICAL_SYSLOG',18843,now());
 test_bug2
---
 1
(1 row)


-- 
Thanks and Regards
Mithun Chicklore Yogendra
EnterpriseDB: http://www.enterprisedb.com

Re: Add extension options to control TAP and isolation tests

[Nikolay Shaplov]

В письме от 10 ноября 2018 09:14:19 пользователь Michael Paquier написал:

> > Nice cleanup. Also, I like the ability to enable/control more types of
> > tests easily from the Makefile. What are the next steps for this
> > patch?
> 
> Thanks.  It seems to me that a complete review is still in order,
> particularly regarding the new makefile option names.  And then, if
> everybody caring about the topic is happy with the way the patch is
> shaped, it can be carried over to being committed, which would be most
> likely something I'll do.

Is it ok, if I join the reviewing? I like test, especially TAP one, you know 
;-)
Since you are much more experienced in postgres then me, I'd try to understand 
how does the patch work, try to use if for writing more TAP test, and will 
report problems and thoughts I came across while doing that.

So far while first reading the patch I came to following two

1. 
--- a/src/test/modules/brin/.gitignore
+++ b/src/test/modules/brin/.gitignore
@@ -1,3 +1,3 @@
 # Generated subdirectories
-/isolation_output/
+/output_iso/
 /tmp_check/

For me name "output_iso" means nothing. iso is something about CD/DVD or about 
standards. I would not guess that iso stands for isolation if I did not know 
it already. isolation_output is more sensible: I have heard that there are 
some isolation tests, this must be something about it. May be it would be 
better to change it to isolation_output everywhere instead of changing to 
output_iso

2.

--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1293,6 +1293,34 @@ include $(PGXS)
   
  
.
+ 
+  ISOLATION
+  
+   
+list of isolation test cases
+   
+  
+ 
+
+ 
+  ISOLATION_OPTS
+  
+   
+additional switches to pass to
+pg_isolation_regress
+   
+  
+ 
+
+ 
+  TAP_TESTS
+  
+   
+switch defining if TAP tests need to be run
+   
+  
+ 
+
  
   NO_INSTALLCHECK
   

I tried to find definition in documentation what does "isolation test" exactly 
means, but did not find. There is some general words about TAP tests in main 
postgres documentation 
https://www.postgresql.org/docs/11/regress-tap.html,
but I would not understand anything from it if I did not already know how it 
works.

In current extend-pgxs documentation there is some explanation about 
regression test, it sensible enough. Since TAP and isolation tests are 
introduced now, there should be same short explanation for both of them.

And also it would be good to add links from extend-pgxs to regress-tap and 
regress saying that for more info about these tests one can look at postgres 
doc, because they work in a similar way.

That's all so far. I'll look more into it later...

-- 
Do code for fun.

signature.asc
Description: This is a digitally signed message part.

Re: [RFC] Removing "magic" oids

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Oct 14, 2018 at 6:35 PM Tom Lane  wrote:
> > Andres Freund  writes:
> > > Does anybody have engineering / architecture level comments about this
> > > proposal?
> >
> > FWIW, I'm -1 on making OIDs be not-magic for SELECT purposes.  Yeah, it's
> > a wart we wouldn't have if we designed the system today, but the wart is
> > thirty years old.  I think changing that will break so many catalog
> > queries that we'll have the villagers on the doorstep.  Most of the other
> > things you're suggesting here could be done easily without making that
> > change.
> >
> > Possibly we could make them not-magic from the storage standpoint (ie
> > they're regular columns) but have a pg_attribute flag that says not
> > to include them in "SELECT *" expansion.
> 
> I think such a flag would be a good idea; it seems to have other uses.
> As Andres also noted, Kevin was quite interested in having a
> hidden-by-default COUNT column to assist with materialized view
> maintenance, and presumably this could also be used for that purpose.

Yeah, I like the idea of having this column too, it'd also be useful for
people who are trying to use column-level privileges.

> I am less sure that it's a good idea that it's a good idea to set that
> flag for the OID columns in system catalogs.  I do think that some
> tool authors are likely to have to do some significant work to update
> their tools, but a lot of tools are probably already querying for
> specific columns, not using *, so they'll be fine.  And like Andres
> says, it seems like we'll be happier in the long term if we get way
> from having OID columns be invisible in system catalogs.  That's just
> confusing.

Tools should *really* be using explicit column names and not just
'SELECT *' (and my experience is that most of them are using explicit
column names in their queries- sometimes to explicitly pull out the oid
column since it's so frequently needed...).  I suppose if we wanted to
get fancy we could allow users to adjust the setting on catalog tables
themselves...  I'd certainly be happier with the OID columns being
visible by SELECT * against the catalog tables though.

By my reckoning, we'll break a lot fewer tools/queries with this change
than the changes we made for xlog -> wal and location -> lsn in various
tables and functions with v10.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [RFC] Removing "magic" oids

[Robert Haas]

On Sun, Oct 14, 2018 at 6:35 PM Tom Lane  wrote:
> Andres Freund  writes:
> > Does anybody have engineering / architecture level comments about this
> > proposal?
>
> FWIW, I'm -1 on making OIDs be not-magic for SELECT purposes.  Yeah, it's
> a wart we wouldn't have if we designed the system today, but the wart is
> thirty years old.  I think changing that will break so many catalog
> queries that we'll have the villagers on the doorstep.  Most of the other
> things you're suggesting here could be done easily without making that
> change.
>
> Possibly we could make them not-magic from the storage standpoint (ie
> they're regular columns) but have a pg_attribute flag that says not
> to include them in "SELECT *" expansion.

I think such a flag would be a good idea; it seems to have other uses.
As Andres also noted, Kevin was quite interested in having a
hidden-by-default COUNT column to assist with materialized view
maintenance, and presumably this could also be used for that purpose.

I am less sure that it's a good idea that it's a good idea to set that
flag for the OID columns in system catalogs.  I do think that some
tool authors are likely to have to do some significant work to update
their tools, but a lot of tools are probably already querying for
specific columns, not using *, so they'll be fine.  And like Andres
says, it seems like we'll be happier in the long term if we get way
from having OID columns be invisible in system catalogs.  That's just
confusing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Can I skip function ResolveRecoveryConflictWithSnapshot if setting hot_standby_feedback=on all the time

[Robert Haas]

On Mon, Nov 19, 2018 at 3:53 AM 范孝剑(康贤)  wrote:
> Hello,
> Can I skip function ResolveRecoveryConflictWithSnapshot if setting 
> hot_standby_feedback=on all the time?
> As I know,  function ResolveRecoveryConflictWithSnapshot is used for 
> resolving conflicts once master cleans dead tuples. But if I set 
> hot_standby_feedback to on, it will not appear conflicts, so I can skip to 
> execute function ResolveRecoveryConflictWithSnapshot, am I right?

If I remember correctly, even with hot_standby_feedback=on, you could
still get conflicts if the connection between the master and the
standby is temporarily broken and then reestablished.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: incorrect xlog.c coverage report

[Alvaro Herrera]

On 2018-Nov-20, Tom Lane wrote:

> Alvaro Herrera  writes:
> > On 2018-Nov-20, Peter Eisentraut wrote:
> >> I noticed some strangeness in the test coverage reporting.
> 
> > Not sure what to make of this.
> 
> What platform and compiler do you run the coverage build on?
> 
> (I'm remembering that gcov was pretty nearly entirely broken on
> Fedora awhile back.  Maybe it's just a little broken on whatever
> you're using.)

This is Debian 9.6.  gcov says:

$ gcov --version
gcov (Debian 6.3.0-18+deb9u1) 6.3.0 20170516

This matches the gcc version string exactly.

ccache is not being used.

configure: using compiler=gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
configure: using CFLAGS=-Wall -Wmissing-prototypes -Wpointer-arith 
-Wdeclaration-after-statement -Werror=vla -Wendif-labels 
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv 
-fexcess-precision=standard -fprofile-arcs -ftest-coverage 
configure: using CPPFLAGS= -D_GNU_SOURCE -I/usr/include/libxml2 
configure: using LDFLAGS=  -Wl,--as-needed


I wondered if perhaps gcov's data files are ending in the wrong place
for some reason, but I don't know how to verify that.  I also wondered
if test results would maybe not be saved for the postmaster executable
when run under the TAP test framework ... not sure about this.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

RE: Shared Memory: How to use SYSV rather than MMAP ?

[REIX, Tony]

Hi Robert,


YES ! Reading this file, your suggestion should work ! Thx !

I've rebuilt and run the basic tests. We'll relaunch our tests asap.

Cordialement,

Tony Reix

tony.r...@atos.net

ATOS / Bull SAS
ATOS Expert
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net

De : Robert Haas 
Envoyé : mardi 20 novembre 2018 15:53:11
À : REIX, Tony
Cc : pgsql-hack...@postgresql.org; EMPEREUR-MOT, SYLVIE
Objet : Re: Shared Memory: How to use SYSV rather than MMAP ?

On Tue, Nov 20, 2018 at 8:36 AM REIX, Tony  wrote:
> We are trying to understand why pgbench on AIX is slower compared to 
> Linux/Power on the same HW/Disks.
>
> So, we have yet no idea about what may be the root cause and what should be 
> changed.
>
> So, changing: dynamic_shared_memory_type = sysvseems to help.
>
> And maybe changing the main shared memory segment could also improve the 
> performance. However, how one can change this?

There's no configuration setting for the main shared memory segment,
but removing #define USE_ANONYMOUS_SHMEM from sysv_shmem.c would
probably do the trick.

--
Robert Haas
EnterpriseDB: 
https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.comdata=01%7C01%7Ctony.reix%40atos.net%7C723ccf057a79436bcf9208d64ef7f48b%7C33440fc6b7c7412cbb730e70b0198d5a%7C0sdata=ZBRv1Ja1THRJH2symVaZSLjGQ4f9hRP9kw27hFlPdAE%3Dreserved=0
The Enterprise PostgreSQL Company

Re: Patch to avoid SIGQUIT accident

[Tom Lane]

Robert Haas  writes:
> Also, sometimes psql gets into a state where it doesn't respond to ^C,
> but ^\ still kills it.  I don't know why that happens, but I've seen
> it repeatedly.

Surely a bug ... please poke into it next time you see it.

regards, tom lane

Re: incorrect xlog.c coverage report

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Nov-20, Peter Eisentraut wrote:
>> I noticed some strangeness in the test coverage reporting.

> Not sure what to make of this.

What platform and compiler do you run the coverage build on?

(I'm remembering that gcov was pretty nearly entirely broken on
Fedora awhile back.  Maybe it's just a little broken on whatever
you're using.)

regards, tom lane

Re: pgbench - doCustom cleanup

[Fabien COELHO]

I didn't quite understand this hunk.  Why does it remove the 
is_latencies conditional?  (The preceding comment shown here should be 
updated obviously if this change is correct, but I'm not sure it is.)


Pgbench runs benches a collects performance data about it.

I simplified the code to always collect data, without trying to be clever 
about cases where these data may not be useful so some collection can be 
skipped.


Here the test avoids recording the statement start time, mostly a simple 
assignment and then later another test avoids recording the stats in the 
same case, which are mostly a few adds.


ISTM that this is over optimization and unlikely to be have any measurable 
effects compared to the other tasks performed when executing commands, so 
a simpler code is better.


--
Fabien.

Re: mysql_fdw crash

[Tom Lane]

066ce...@free.fr writes:
> What is confusing, is that if I do the same with a pl/pgsql function (see 
> below) I can run it 5 times, and the 6th exec hit the same SEGV...

That probably reflects switching from a custom plan to a generic plan
on the sixth execution.

regards, tom lane

Re: Patch to avoid SIGQUIT accident

[Robert Haas]

On Sun, Oct 21, 2018 at 7:21 PM Tom Lane  wrote:
> * SIGQUIT is a fairly well-known way to get out of an application when all
> else fails.  People who aren't familiar with psql's exit commands might
> find it pretty unfriendly of us to block this off.

Also, sometimes psql gets into a state where it doesn't respond to ^C,
but ^\ still kills it.  I don't know why that happens, but I've seen
it repeatedly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Robert Haas]

On Tue, Nov 20, 2018 at 8:36 AM REIX, Tony  wrote:
> We are trying to understand why pgbench on AIX is slower compared to 
> Linux/Power on the same HW/Disks.
>
> So, we have yet no idea about what may be the root cause and what should be 
> changed.
>
> So, changing: dynamic_shared_memory_type = sysvseems to help.
>
> And maybe changing the main shared memory segment could also improve the 
> performance. However, how one can change this?

There's no configuration setting for the main shared memory segment,
but removing #define USE_ANONYMOUS_SHMEM from sysv_shmem.c would
probably do the trick.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [RFC] Removing "magic" oids

[John Naylor]

On 11/20/18, Andres Freund  wrote:
> I'm
> sure we'll find some things to adapt around the margins, but imo the
> patch as a whole looks pretty reasonable.

bki.sgml is now a bit out of date. I've attached a draft fix.

-John Naylor
diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
index 0fb309a1bd..1919bac936 100644
--- a/doc/src/sgml/bki.sgml
+++ b/doc/src/sgml/bki.sgml
@@ -89,7 +89,7 @@
The CATALOG line can also be annotated, with some
other BKI property macros described in genbki.h, to
define other properties of the catalog as a whole, such as whether
-   it has OIDs (by default, it does).
+   it is a shared relation.
   
 
   
@@ -354,7 +354,7 @@
 also needed if the row's OID must be referenced from C code.
 If neither case applies, the oid metadata field can
 be omitted, in which case the bootstrap code assigns an OID
-automatically, or leaves it zero in a catalog that has no OIDs.
+automatically.
 In practice we usually preassign OIDs for all or none of the pre-loaded
 rows in a given catalog, even if only some of them are actually
 cross-referenced.
@@ -387,15 +387,16 @@
 through the catalog headers and .dat files
 to see which ones do not appear.  You can also use
 the duplicate_oids script to check for mistakes.
-(genbki.pl will also detect duplicate OIDs
+(genbki.pl will assign OIDs for any rows that
+didn't get one hand-assigned to them and also detect duplicate OIDs
 at compile time.)

 

 The OID counter starts at 1 at the beginning of a bootstrap run.
-If a catalog row is in a table that requires OIDs, but no OID was
-preassigned by an oid field, then it will
-receive an OID of 1 or above.
+If a row from a source other than postgres.bki
+is inserted into a table that requires OIDs, then it will receive an
+OID of 1 or above.

   
 
@@ -714,7 +715,6 @@ $ perl  rewrite_dat_with_prokind.pl  pg_proc.dat
  tableoid
  bootstrap
  shared_relation
- without_oids
  rowtype_oid oid
  (name1 =
  type1
@@ -766,7 +766,6 @@ $ perl  rewrite_dat_with_prokind.pl  pg_proc.dat
  
   The table is created as shared if shared_relation is
   specified.
-  It will have OIDs unless without_oids is specified.
   The table's row type OID (pg_type OID) can optionally
   be specified via the rowtype_oid clause; if not specified,
   an OID is automatically generated for it.  (The rowtype_oid
@@ -805,7 +804,7 @@ $ perl  rewrite_dat_with_prokind.pl  pg_proc.dat
 

 
- insert OID = oid_value ( value1 value2 ... )
+ insert OID = oid_value ( oid_value value1 value2 ... )
 
 
 
@@ -813,11 +812,10 @@ $ perl  rewrite_dat_with_prokind.pl  pg_proc.dat
   Insert a new row into the open table using value1, value2, etc., for its column
-  values and oid_value for its OID.  If
-  oid_value is zero
-  (0) or the clause is omitted, and the table has OIDs, then the
-  next available OID is assigned.
+  values and oid_value
+  for its OID. Note that OID is stored in an ordinary column; the
+  OID = oid_value
+  statement is only there for debugging purposes.
  
 
  
@@ -991,10 +989,10 @@ $ perl  rewrite_dat_with_prokind.pl  pg_proc.dat
int4 and text, respectively, and insert
two rows into the table:
 
-create test_table 420 (cola = int4, colb = text)
+create test_table 420 (oid = oid, cola = int4, colb = text)
 open test_table
-insert OID=421 ( 1 "value1" )
-insert OID=422 ( 2 _null_ )
+insert OID=421 ( 421 1 "value1" )
+insert OID=422 ( 422 2 _null_ )
 close test_table
 
   

Re: Psql patch to show access methods info

[Arthur Zakirov]

Hello,

On 20.11.2018 16:08, s.cherkas...@postgrespro.ru wrote:

Ok, I fixed this.


I looked at the patch. It is in good shape. It compiles and tests are 
passed.


I have few a questions related with throwing errors. They might be silly :)

\dAp as well as \dA command throw an error if a server's version below 9.6:

"The server (version %s) does not support access methods"

But other \dA commands don't. It seems that there is enough information 
in catalog for servers below 9.6. That is there are pg_am, pg_opfamily, 
pg_amop and other catalog tables related with access methods.


\dAp calls pg_indexam_has_property() function, which doesn't exist in 
servers 9.5 and below. Is this the reason that it throws an error? If so 
then describeOneIndexColumnProperties() also should throw an error, 
because it calls pg_index_column_has_property() function, which doesn't 
exist in servers 9.5 and below.


What do you think?

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: mysql_fdw crash

[Tomas Vondra]

On 11/20/18 3:06 PM, 066ce...@free.fr wrote:

Hi,


When gdb will be active, then use command c, and then run query in session. gdb 
should to catch segfault.


Thank you very much. It's been helpfull.

BTW behaviour is strange. When I'm executing following, I do have always a SEGV 
:

psql (11.1)
Type "help" for help.

herve=# CREATE OR REPLACE FUNCTION public.test_bug2(text,integer,timestamp with 
time zone)
herve-#  RETURNS integer
herve-#
herve-# AS '
herve'#
herve'# select coalesce(max(id),1) from sact_v1.autocalls where  label=$1 
and machine_id=$2 and created_date=$3;
herve'# '
herve-#  LANGUAGE sql;
CREATE FUNCTION
herve=# select test_bug2('BSM_CRITICAL_SYSLOG',18843,now());

The GDB session :

Continuing.

Program received signal SIGSEGV, Segmentation fault.
prepare_query_params (param_types=0x1c86ac8, param_values=0x1c86ac0, 
param_exprs=0x1c86ab8, param_flinfo=0x1c86ab0, numParams=3, 
fdw_exprs=0x1c6b5b8, node=0x1c792d8) at mysql_fdw.c:2139
2139*param_types[i] = exprType(param_expr);
(gdb) bt


So which part of that expression triggers the segfault? Try printing the 
different parts, i.e.


p i
p param_types[i]

It might be helpful to also install the debug package, which would give 
us more readable backtraces.


BTW, considering the failure is in mysql_fdw.c, this very much seems 
like a bug in mysql_fdw - have you tried reporting it through the 
project github repository?


https://github.com/EnterpriseDB/mysql_fdw/issues

That's probably more likely to help, and even if we find a bug here we 
can't really commit that (perhaps some of the mysql_fdw authors are 
watching this list, but I'm not sure about that).


regards

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

Re: pgbench - doCustom cleanup

[Alvaro Herrera]

On 2018-Nov-20, Fabien COELHO wrote:

> Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
> transitions to happen in doCustom's switch (st->state) and nowhere else,
> which is defeated by creating the separate function.
> 
> Although it improves readability at one level, it does not help figuring out
> what happens to states, which is my primary concern: The idea is that
> reading doCustom is enough to build and check the automaton, which I had to
> do repeatedly while reviewing Marina's patches.

Yeah, there are conflicting goals here.


I didn't quite understand this hunk.  Why does it remove the
is_latencies conditional?  (The preceding comment shown here should be
updated obviously if this change is correct, but I'm not sure it is.)

@@ -3364,42 +3334,34 @@ doCustom(TState *thread, CState *st, StatsData *agg)
/*
 * command completed: accumulate per-command 
execution times
 * in thread-local data structure, if 
per-command latencies
 * are requested.
 */
-   if (is_latencies)
-   {
-   if (INSTR_TIME_IS_ZERO(now))
-   INSTR_TIME_SET_CURRENT(now);
+   INSTR_TIME_SET_CURRENT_LAZY(now);
 
-   /* XXX could use a mutex here, but we 
choose not to */
-   command = 
sql_script[st->use_file].commands[st->command];
-   addToSimpleStats(>stats,
-
INSTR_TIME_GET_DOUBLE(now) -
-
INSTR_TIME_GET_DOUBLE(st->stmt_begin));
-   }
+   /* XXX could use a mutex here, but we choose 
not to */
+   command = 
sql_script[st->use_file].commands[st->command];
+   addToSimpleStats(>stats,
+
INSTR_TIME_GET_DOUBLE(now) -
+
INSTR_TIME_GET_DOUBLE(st->stmt_begin));


-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Undo logs

[Dilip Kumar]

On Sat, Nov 17, 2018 at 5:12 PM Amit Kapila  wrote:
>
> On Fri, Nov 16, 2018 at 9:46 AM Dilip Kumar  wrote:
> >
> > On Thu, Nov 15, 2018 at 12:14 PM Dilip Kumar  wrote:
> > >
> > Updated patch (merged latest code from the zheap main branch [1]).
> >
>
> Review comments:
> ---
> 1.
> UndoRecordPrepareTransInfo()
> {
> ..
> + /*
> + * The absence of previous transaction's undo indicate that this backend
> + * is preparing its first undo in which case we have nothing to update.
> + */
> + if (!UndoRecPtrIsValid(prev_xact_urp))
> + return;
> ..
> }
>
> It is expected that the caller of UndoRecPtrIsValid should have
> discard lock, but I don't see that how this the call from this place
> ensures the same?


I think its duplicate code, made mistake while merging from the zheap branch
>
>
> 2.
> UndoRecordPrepareTransInfo()
> {
> ..
> /*
> + * The absence of previous transaction's undo indicate that this backend
> + * is preparing its first undo in which case we have nothing to update.
> + */
> + if (!UndoRecPtrIsValid(prev_xact_urp))
> + return;
> +
> + /*
> + * Acquire the discard lock before accessing the undo record so that
> + * discard worker doesn't remove the record while we are in process of
> + * reading it.
> + */
> + LWLockAcquire(>discard_lock, LW_SHARED);
> +
> + if (!UndoRecordIsValid(log, prev_xact_urp))
> + return;
> ..
> }
>
> I don't understand this logic where you are checking the same
> information with and without a lock, is there any reason for same?  It
> seems we don't need the first call to  UndoRecPtrIsValid is not
> required.


Removed
>
>
> 3.
> UndoRecordPrepareTransInfo()
> {
> ..
> + while (true)
> + {
> + bufidx = InsertFindBufferSlot(rnode, cur_blk,
> +   RBM_NORMAL,
> +   log->meta.persistence);
> + prev_txn_info.prev_txn_undo_buffers[index] = bufidx;
> + buffer = undo_buffer[bufidx].buf;
> + page = BufferGetPage(buffer);
> + index++;
> +
> + if (UnpackUndoRecord(_txn_info.uur, page, starting_byte,
> + _decoded, true))
> + break;
> +
> + starting_byte = UndoLogBlockHeaderSize;
> + cur_blk++;
> + }
>
>
> Can you write some commentary on what this code is doing?

Done
>
>
> There is no need to use index++; as a separate statement, you can do
> it while assigning the buffer in that index.

Done
>
>
> 4.
> +UndoRecordPrepareTransInfo(UndoRecPtr urecptr, bool log_switched)
> +{
> + UndoRecPtr prev_xact_urp;
>
> I think you can simply name this variable as xact_urp.  All this and
> related prev_* terminology used for variables seems confusing to me. I
> understand that you are trying to update the last transactions undo
> record information, but you can explain that via comments.  Keeping
> such information as part of variable names not only makes their length
> longer, but is also confusing.
>
> 5.
> /*
> + * Structure to hold the previous transaction's undo update information.
> + */
> +typedef struct PreviousTxnUndoRecord
> +{
> + UndoRecPtr prev_urecptr; /* prev txn's starting urecptr */
> + int prev_txn_undo_buffers[MAX_BUFFER_PER_UNDO];
> + UnpackedUndoRecord uur; /* prev txn's first undo record. */
> +} PreviousTxnInfo;
> +
> +static PreviousTxnInfo prev_txn_info;
>
> Due to reasons mentioned in point-4, lets name the structure and it's
> variables as below:
>
> typedef struct XactUndoRecordInfo
> {
> UndoRecPtr start_urecptr; /* prev txn's starting urecptr */
> int idx_undo_buffers[MAX_BUFFER_PER_UNDO];
> UnpackedUndoRecord first_uur; /* prev txn's first undo record. */
> } XactUndoRecordInfo;
>
> static XactUndoRecordInfo xact_ur_info;


Done,  but I have kept start_urecptr as urecptr and first_uur as uur
and explained in comment.
>
>
> 6.
> +static int
> +InsertFindBufferSlot(RelFileNode rnode,
>
> The name of this function is not clear, can we change it to
> UndoGetBufferSlot or UndoGetBuffer?


Changed to UndoGetBufferSlot
>
>
> 7.
> +UndoRecordAllocateMulti(UnpackedUndoRecord *undorecords, int nrecords,
> + UndoPersistence upersistence, TransactionId txid)
> {
> ..
> + /*
> + * If this is the first undo record for this transaction then set the
> + * uur_next to the SpecialUndoRecPtr.  This is the indication to allocate
> + * the space for the transaction header and the valid value of the uur_next
> + * will be updated while preparing the first undo record of the next
> + * transaction.
> + */
> + first_rec_in_recovery = InRecovery && IsTransactionFirstRec(txid);
> ..
> }


Done
>
>
> I think it will be better if we move this comment few lines down:
> + if (need_start_undo && i == 0)
> + {
> + urec->uur_next = SpecialUndoRecPtr;
>
> BTW, is the only reason set a special value (SpecialUndoRecPtr) for
> uur_next is for allocating transaction header? If so, can't we
> directly set the corresponding flag (UREC_INFO_TRANSACTION) in
> uur_info and then remove it from UndoRecordSetInfo?


yeah, Done that way.
>
>
> I think it would have been better if there is one central location to
> set uur_info, but as that is becoming tricky,
> we should not try 

Re: mysql_fdw crash

[066ce286]

Hi,


>When gdb will be active, then use command c, and then run query in session. 
>gdb should to catch segfault. 



Thank you very much. It's been helpfull.

BTW behaviour is strange. When I'm executing following, I do have always a SEGV 
:

psql (11.1)
Type "help" for help.

herve=# CREATE OR REPLACE FUNCTION public.test_bug2(text,integer,timestamp with 
time zone)
herve-#  RETURNS integer
herve-# 
herve-# AS '
herve'# 
herve'# select coalesce(max(id),1) from sact_v1.autocalls where  label=$1 
and machine_id=$2 and created_date=$3;
herve'# '
herve-#  LANGUAGE sql;
CREATE FUNCTION
herve=# select test_bug2('BSM_CRITICAL_SYSLOG',18843,now());

The GDB session :

Continuing.

Program received signal SIGSEGV, Segmentation fault.
prepare_query_params (param_types=0x1c86ac8, param_values=0x1c86ac0, 
param_exprs=0x1c86ab8, param_flinfo=0x1c86ab0, numParams=3, 
fdw_exprs=0x1c6b5b8, node=0x1c792d8) at mysql_fdw.c:2139
2139*param_types[i] = exprType(param_expr);
(gdb) bt
#0  prepare_query_params (param_types=0x1c86ac8, param_values=0x1c86ac0, 
param_exprs=0x1c86ab8, param_flinfo=0x1c86ab0, numParams=3, 
fdw_exprs=0x1c6b5b8, node=0x1c792d8)
at mysql_fdw.c:2139
#1  mysqlBeginForeignScan (node=0x1c792d8, eflags=) at 
mysql_fdw.c:503
#2  0x0062ae94 in ExecInitForeignScan ()
#3  0x006077bf in ExecInitNode ()
#4  0x0061117d in ExecInitAgg ()
#5  0x00607717 in ExecInitNode ()
#6  0x00601cf4 in standard_ExecutorStart ()
#7  0x0060d6ec in fmgr_sql ()
#8  0x005fd504 in ExecInterpExpr ()
#9  0x006258fb in ExecResult ()
#10 0x006009aa in standard_ExecutorRun ()
#11 0x0073eaec in PortalRunSelect ()
#12 0x0073fede in PortalRun ()
#13 0x0073bd82 in exec_simple_query ()
#14 0x0073d249 in PostgresMain ()
#15 0x0047cff6 in ServerLoop ()
#16 0x006cf7b3 in PostmasterMain ()
#17 0x0047ded1 in main ()

What is confusing, is that if I do the same with a pl/pgsql function (see 
below) I can run it 5 times, and the 6th exec hit the same SEGV...


CREATE OR REPLACE FUNCTION public.test_bug(text,text)
 RETURNS integer
 LANGUAGE plpgsql
AS $function$
DECLARE
plabel ALIAS FOR $1;
spmachine_id ALIAS FOR $2;
rid integer;
lnow timestamp with time zone;
pmachine_id INTEGER;

BEGIN
pmachine_id := cast(spmachine_id as INTEGER);
lnow:=now();

select max(id) into rid from sact_v1.autocalls where  label=plabel and 
machine_id=pmachine_id and created_date=lnow;
rid := coalesce(rid,-1);

return  rid;
END;
$function$;

CREATE FUNCTION
herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');
 test_bug 
--
   -1
(1 row)

herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');
 test_bug 
--
   -1
(1 row)

herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');
 test_bug 
--
   -1
(1 row)

herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');
 test_bug 
--
   -1
(1 row)

herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');
 test_bug 
--
   -1
(1 row)

herve=# select test_bug('BSM_CRITICAL_SYSLOG','18843');



Program received signal SIGSEGV, Segmentation fault.
prepare_query_params (param_types=0x1ca3558, param_values=0x1ca3550, 
param_exprs=0x1ca3548, param_flinfo=0x1ca3540, numParams=3, 
fdw_exprs=0x1ca8638, node=0x1cade28) at mysql_fdw.c:2139
2139*param_types[i] = exprType(param_expr);
(gdb) bt
#0  prepare_query_params (param_types=0x1ca3558, param_values=0x1ca3550, 
param_exprs=0x1ca3548, param_flinfo=0x1ca3540, numParams=3, 
fdw_exprs=0x1ca8638, node=0x1cade28)
at mysql_fdw.c:2139
#1  mysqlBeginForeignScan (node=0x1cade28, eflags=) at 
mysql_fdw.c:503
#2  0x0062ae94 in ExecInitForeignScan ()
#3  0x006077bf in ExecInitNode ()
#4  0x0061117d in ExecInitAgg ()
#5  0x00607717 in ExecInitNode ()
#6  0x00601cf4 in standard_ExecutorStart ()
#7  0x00632946 in _SPI_execute_plan ()
#8  0x00632d0b in SPI_execute_plan_with_paramlist ()
#9  0x7ffb349aba22 in exec_stmt_execsql () from 
/usr/local/pgsql/lib/plpgsql.so
#10 0x7ffb349ace43 in exec_stmts () from /usr/local/pgsql/lib/plpgsql.so
#11 0x7ffb349af6d3 in exec_stmt_block () from 
/usr/local/pgsql/lib/plpgsql.so
#12 0x7ffb349af88f in plpgsql_exec_function () from 
/usr/local/pgsql/lib/plpgsql.so
#13 0x7ffb349a3375 in plpgsql_call_handler () from 
/usr/local/pgsql/lib/plpgsql.so
#14 0x005fd504 in ExecInterpExpr ()
#15 0x006258fb in ExecResult ()
#16 0x006009aa in standard_ExecutorRun ()
#17 0x0073eaec in PortalRunSelect ()
#18 0x0073fede in PortalRun ()
#19 0x0073bd82 in exec_simple_query ()
#20 0x0073d249 in PostgresMain ()
#21 0x0047cff6 in ServerLoop ()
#22 0x006cf7b3 in PostmasterMain ()
#23 0x0047ded1 in main ()

Re: Psql patch to show access methods info

[Alvaro Herrera]

On 2018-Nov-20, s.cherkas...@postgrespro.ru wrote:

> Ok, I fixed this.

Cool.  I'm not sure this is a good idea: "c.relname::pg_catalog.regclass"
I would use c.oid::pg_catalog.regclass instead.

But before getting into those details, I think we should discuss the
user interface that this patch is offering:

\dip [am pattern]
  lists index properties (according to doc patch)
  * OK, but why do we need an AM pattern?  ... reads regress output  ...
oh, actually it's an index name pattern, not an AM pattern. Please fix docs.

\dicp [idx pattern] [column pattern]
  list index column properties
  * I think the column pattern part is pointless.

\dA{f,p,fo,fp,oc}
  Please explain what these are.

I think this is two patches -- one being the \dip/\dicp part, the other
the \dA additions.  Let's deal with them separately?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgbench - doCustom cleanup

[Fabien COELHO]

Hello Alvaro,

Thanks for the review and improvements.


Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e


Attached v5.  I thought that separating the part that executes the
command was an obvious readability improvement.


Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state 
transitions to happen in doCustom's switch (st->state) and nowhere else, 
which is defeated by creating the separate function.


Although it improves readability at one level, it does not help figuring 
out what happens to states, which is my primary concern: The idea is that 
reading doCustom is enough to build and check the automaton, which I had 
to do repeatedly while reviewing Marina's patches.


Also the added doCustom comment, which announces this property becomes 
false under the refactoring function:


All state changes are performed within this function called by 
threadRun.

So I would suggest not to create this function.


Do we really use the word "move" to talk about state changes?  It sounds
very odd to me.  I would change that to "transition" -- would anybody
object to that?  (Not changed in v5.)


Yep. I removed the goto:-)


On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
macro -- consider this:
if (foo)
INSTR_TIME_SET_CURRENT_LAZY(bar);
else
something_else();
Which "if" is the else now attached to?  Now maybe the C standard has an
answer for that (I don't know what it is), but it's hard to read and
likely the compiler will complain anyway.  I wrapped it in "do { }
while(0)" as is customary.


Indeed, good catch.

In the attached patched, I have I think included all your changes *but* 
the separate function, for the reason discussed above, and removed the 
"goto".


--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 73d3de0677..82cbd20420 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -294,24 +294,32 @@ typedef enum
 {
 	/*
 	 * The client must first choose a script to execute.  Once chosen, it can
-	 * either be throttled (state CSTATE_START_THROTTLE under --rate) or start
-	 * right away (state CSTATE_START_TX).
+	 * either be throttled (state CSTATE_START_THROTTLE under --rate), start
+	 * right away (state CSTATE_START_TX) or not start at all if the timer was
+	 * exceeded (state CSTATE_FINISHED).
 	 */
 	CSTATE_CHOOSE_SCRIPT,
 
 	/*
 	 * In CSTATE_START_THROTTLE state, we calculate when to begin the next
 	 * transaction, and advance to CSTATE_THROTTLE.  CSTATE_THROTTLE state
-	 * sleeps until that moment.  (If throttling is not enabled, doCustom()
-	 * falls directly through from CSTATE_START_THROTTLE to CSTATE_START_TX.)
+	 * sleeps until that moment.
+	 *
+	 * It may also detect that the next transaction would start beyond the end
+	 * of run, and switch to CSTATE_FINISHED.
 	 */
 	CSTATE_START_THROTTLE,
 	CSTATE_THROTTLE,
 
 	/*
 	 * CSTATE_START_TX performs start-of-transaction processing.  Establishes
-	 * a new connection for the transaction, in --connect mode, and records
-	 * the transaction start time.
+	 * a new connection for the transaction in --connect mode, records
+	 * the transaction start time, and proceed to the first command.
+	 *
+	 * Note: once a script is started, it will either error or run till
+	 * its end, where it may be interrupted. It is not interrupted while
+	 * running, so pgbench --time is to be understood as tx are allowed to
+	 * start in that time, and will finish when their work is completed.
 	 */
 	CSTATE_START_TX,
 
@@ -324,9 +332,6 @@ typedef enum
 	 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 	 * meta-commands are executed immediately.
 	 *
-	 * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
-	 * quickly skip commands that do not need any evaluation.
-	 *
 	 * CSTATE_WAIT_RESULT waits until we get a result set back from the server
 	 * for the current command.
 	 *
@@ -334,19 +339,25 @@ typedef enum
 	 *
 	 * CSTATE_END_COMMAND records the end-of-command timestamp, increments the
 	 * command counter, and loops back to CSTATE_START_COMMAND state.
+	 *
+	 * CSTATE_SKIP_COMMAND is used by conditional branches which are not
+	 * executed. It quickly skip commands that do not need any evaluation.
+	 * This state can move forward several commands, till there is something
+	 * to do or the end of the script.
 	 */
 	CSTATE_START_COMMAND,
-	CSTATE_SKIP_COMMAND,
 	CSTATE_WAIT_RESULT,
 	CSTATE_SLEEP,
 	CSTATE_END_COMMAND,
+	CSTATE_SKIP_COMMAND,
 
 	/*
-	 * CSTATE_END_TX performs end-of-transaction processing.  Calculates
-	 * latency, and logs the transaction.  In --connect mode, closes the
-	 * current connection.  Chooses the next script to execute and starts over
-	 * in CSTATE_START_THROTTLE state, or enters CSTATE_FINISHED if we have no
-	 * more work to do.
+	 * CSTATE_END_TX performs end-of-transaction processing.  It calculates
+	 * 

RE: Shared Memory: How to use SYSV rather than MMAP ?

[REIX, Tony]

Hi Robert,


We are trying to understand why pgbench on AIX is slower compared to 
Linux/Power on the same HW/Disks.

So, we have yet no idea about what may be the root cause and what should be 
changed.


So, changing: dynamic_shared_memory_type = sysvseems to help.

And maybe changing the main shared memory segment could also improve the 
performance. However, how one can change this?


Cordialement,

Tony Reix

tony.r...@atos.net

ATOS / Bull SAS
ATOS Expert
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net

De : Robert Haas 
Envoyé : mardi 20 novembre 2018 13:53:53
À : REIX, Tony
Cc : pgsql-hack...@postgresql.org; EMPEREUR-MOT, SYLVIE
Objet : Re: Shared Memory: How to use SYSV rather than MMAP ?

On Tue, Nov 20, 2018 at 5:11 AM REIX, Tony  wrote:
> On AIX, since with MMAP we have only 4K pages though we can have 64K pages 
> with SYSV, we'd like to experiment with SYSV rather than MMAP and measure the 
> impact to the performance.

Are you trying to move the main shared memory segment or the dynamic
shared memory segments?

--
Robert Haas
EnterpriseDB: 
https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.comdata=01%7C01%7Ctony.reix%40atos.net%7C09c690fe81b9489e135d08d64ee74b7f%7C33440fc6b7c7412cbb730e70b0198d5a%7C0sdata=oj%2Fd7djWk16Bb8%2F2I9eiqlWnRBfcFNjYtZCj%2FHd3Qp0%3Dreserved=0
The Enterprise PostgreSQL Company

Re: Regarding performance regression on specific query

[Amit Langote]

On Tue, Nov 20, 2018 at 10:08 PM Jung, Jinho  wrote:
> We are wondering how ANALYZE mitigated regression from query "1.sql" and 
> "4.sql".
>
> We followed this procedure but still observe performance regression:
> 1) run ANALYZE on used table_name
> analyze pg_catalog.pg_ts_parser;
> analyze information_schema.column_options;
> analyze pg_catalog.pg_aggregate;
> analyze pg_catalog.pg_inherits;
> analyze pg_catalog.pg_aggregate;
> analyze pg_catalog.pg_rewrite;
> analyze pg_catalog.pg_stat_user_indexes;
> analyze pg_catalog.pg_stat_user_tables;
> analyze pg_catalog.pg_attribute;
> analyze information_schema.column_privileges;
> analyze pg_catalog.pg_user_mapping;
> analyze pg_catalog.pg_type;
> analyze pg_catalog.pg_shseclabel;
> analyze pg_catalog.pg_statio_sys_sequences;
> analyze information_schema.role_routine_grants;
> analyze pg_catalog.pg_type;
> analyze information_schema.user_mapping_options;
> analyze pg_catalog.pg_stat_xact_sys_tables;

You can run ANALYZE without explicitly specifying any table name, so
that you don't miss any.

> We have more cases. Do you think we should report them through the bug report 
> website? (https://www.postgresql.org/account/login/?next=/account/submitbug/)

You can send an email to pgsql-performance mailing list.

https://www.postgresql.org/list/pgsql-performance/

Thanks,
Amit

Re: Psql patch to show access methods info

[s . cherkashin]

Ok, I fixed this.

On 2018-11-20 13:41, Alvaro Herrera wrote:

On 2018-Nov-20, s.cherkas...@postgrespro.ru wrote:


Yes, I am available to finish this patch.
I’m sorry that I hadn’t updated patch for new commitfest and I 
grateful to

you for doing it and fixing some issues.
I would like to clarify which commands lack the output of the schema 
names?

Because I tried to display them for all objects that have a schema.


I think Michael is referring to the queries used to obtain the data.
For example "FROM pg_class c" is bogus -- it must be "FROM
pg_catalog.pg_class c".
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 8b7f169d50..565b1c396a 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -675,7 +675,7 @@
search and ordering purposes.)
   
 
-  
+  
pg_amop Columns
 

@@ -818,7 +818,7 @@
is one row for each support function belonging to an operator family.
   
 
-  
+  
pg_amproc Columns
 

@@ -4430,7 +4430,7 @@ SCRAM-SHA-256$iteration count:
Operator classes are described at length in .
   
 
-  
+  
pg_opclass Columns
 

@@ -4692,7 +4692,7 @@ SCRAM-SHA-256$iteration count:
Operator families are described at length in .
   
 
-  
+  
pg_opfamily Columns
 

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 6e6d0f42d1..7f2631d75d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1203,6 +1203,95 @@ testdb=
 
 
   
+  
+  
+
+  \dAf
+  [ access-method-pattern 
+[operator-family-pattern]]
+  
+
+
+
+
+Lists operator families (). If access-method-pattern is specified, only
+families whose access method name matches the pattern are shown.
+If operator-family-pattern is specified, only
+opereator families associated with whose name matches the pattern are shown.
+If + is appended to the command name, each operator
+family is listed with it's owner.
+
+
+  
+
+  
+
+  \dAfo
+[access-method-pattern
+  [operator-family-pattern]]
+  
+
+
+
+
+Lists operators () associated with access method operator families.
+If access-method-patttern is specified,
+only operators associated with access method whose name matches pattern are shown.
+If operator-family-pattern is specified, only
+opereators associated with families whose name matches the pattern are shown.
+
+
+  
+
+  
+
+  \dAfp
+[access-method-pattern
+  [operator-family-pattern]]
+  
+
+
+
+List procedures () accociated with access method operator families.
+If access-method-patttern is specified,
+only procedures associated with access method whose name matches pattern are shown.
+If operator-family-pattern is specified, only
+procedures associated with families whose name matches the pattern are shown.
+
+
+  
+
+  
+
+  \dAop
+[access-method-pattern
+  [operator-class-pattern]]
+  
+
+
+
+Shows index access method operator classes listed in .
+If access-method-patttern is specified,
+only operator classes associated with access method whose name matches pattern are shown.
+If operator-class-pattern is specified, only
+procedures associated with families whose name matches the pattern are shown.
+
+
+  
+
+
+  
+\dAp [ pattern ]
+
+
+
+Shows access method properties listed in . If pattern is specified, only access
+methods whose names match the pattern are shown.
+
+
+  
 
   
 \db[+] [ pattern ]
@@ -1351,6 +1440,35 @@ testdb=
 
   
 
+  
+\dip [ pattern ]
+
+
+
+Shows index properties listed in . If pattern is specified, only access
+methods whose names match the pattern are shown.
+
+
+  
+
+  
+
+  \dicp [ index-name-pattern
+  [ column-name-pattern ]]
+  
+
+
+
+
+Shows index column properties listed in . If column_name is specified, only column
+with such name is shown.
+
+
+  
 
   
 \des[+] [ pattern ]
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 04e227b5a6..0043e37040 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -703,7 +703,23 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
 	success = listTables("tvmsE", NULL, show_verbose, show_system);
 break;
 			case 'A':

Re: Regarding performance regression on specific query

[Jung, Jinho]

Thanks for the test.


We are wondering how ANALYZE mitigated regression from query "1.sql" and 
"4.sql".

We followed this procedure but still observe performance regression:


1) run ANALYZE on used table_name

analyze pg_catalog.pg_ts_parser;
analyze information_schema.column_options;
analyze pg_catalog.pg_aggregate;
analyze pg_catalog.pg_inherits;
analyze pg_catalog.pg_aggregate;
analyze pg_catalog.pg_rewrite;
analyze pg_catalog.pg_stat_user_indexes;
analyze pg_catalog.pg_stat_user_tables;
analyze pg_catalog.pg_attribute;
analyze information_schema.column_privileges;
analyze pg_catalog.pg_user_mapping;
analyze pg_catalog.pg_type;
analyze pg_catalog.pg_shseclabel;
analyze pg_catalog.pg_statio_sys_sequences;
analyze information_schema.role_routine_grants;
analyze pg_catalog.pg_type;
analyze information_schema.user_mapping_options;
analyze pg_catalog.pg_stat_xact_sys_tables;

2) execute the same query



We have more cases. Do you think we should report them through the bug report 
website? (https://www.postgresql.org/account/login/?next=/account/submitbug/)


Jinho Jung


From: Amit Langote 
Sent: Tuesday, November 20, 2018 2:47:54 AM
To: Jung, Jinho; pgsql-hack...@postgresql.org
Subject: Re: Regarding performance regression on specific query

Hi,

On 2018/11/20 2:49, Jung, Jinho wrote:
> Execution time
> =
> 1.sql
> 10.6  : 469 ms
> 9.4.20: 10 ms
>
> 4.sql
> 10.6  : 34019 ms
> 9.4.20: 0.4 ms

I noticed that these two are fixed by running ANALYZE in the database in
which these queries are run.

> 20.sql
> 10.6  : 2791 ms
> 9.4.20: 61 ms

This one may be suffering from a more serious planning issue, as doing
ANALYZE didn't help for this one.  Will have to look closely at how the
plan is changing for worse.

Thanks,
Amit

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Robert Haas]

On Tue, Nov 20, 2018 at 5:11 AM REIX, Tony  wrote:
> On AIX, since with MMAP we have only 4K pages though we can have 64K pages 
> with SYSV, we'd like to experiment with SYSV rather than MMAP and measure the 
> impact to the performance.

Are you trying to move the main shared memory segment or the dynamic
shared memory segments?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Tue, Nov 20, 2018 at 1:42 PM John Naylor  wrote:
>
> I wrote:
>
> > On 11/19/18, Amit Kapila  wrote:
> > [ abortive states ]
> >> I think it might come from any other place between when you set it and
> >> before it got cleared (like any intermediate buffer and pin related
> >> API's).
> >
> > Okay, I will look into that.
>
> LockBuffer(), visibilitymap_pin(), and GetVisibilityMapPins() don't
> call errors at this level. I don't immediately see any additional good
> places from which to clear the local map.
>

LockBuffer()->LWLockAcquire() can error out.  Similarly,
ReadBuffer()->ReadBufferExtended() and calls below it can error ou.
To handle them, you need to add a call to clear local map in
Abortransaction code path.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: wal_dump output on CREATE DATABASE

[Jean-Christophe Arnu]

Le mar. 20 nov. 2018 à 13:34, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> a écrit :

> On 16/11/2018 17:28, Jean-Christophe Arnu wrote:
> > On the other hand, there's a consensus not to go further than the
> > initial patch.
>
> I have committed your original patch.
>
> Great, thank you Peter. And thank you all for that interesting discussion.
Regards

-- 
Jean-Christophe Arnu

Re: wal_dump output on CREATE DATABASE

[Peter Eisentraut]

On 16/11/2018 17:28, Jean-Christophe Arnu wrote:
> On the other hand, there's a consensus not to go further than the
> initial patch.

I have committed your original patch.

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

Re: incorrect xlog.c coverage report

[Alvaro Herrera]

On 2018-Nov-20, Peter Eisentraut wrote:

> I noticed some strangeness in the test coverage reporting.  For example,
> in
> https://coverage.postgresql.org/src/backend/access/transam/xlog.c.gcov.html
> in the function readRecoveryCommandFile(), most of the branches parsing
> the individual recovery options (recovery_target_xid,
> recovery_target_time, etc.) are shown as never hit, even though there
> are explicit tests for this in
> src/test/recovery/t/003_recovery_targets.pl.  I tried this locally and
> also with -O0 just in case, but I get the same results.  Any ideas?

I've posted this before, but as a reminder, the coverage script does this:

  ./configure --enable-depend --enable-coverage --enable-tap-tests --enable-nls 
--with-python --with-perl --with-tcl --with-openssl --with-libxml --with-ldap 
--with-pam >> $LOG 2>&1

  # run tests
  make -j4 >> $LOG 2>&1
  make -j4 -C contrib >> $LOG 2>&1
  make check-world PG_TEST_EXTRA="ssl ldap" >> $LOG 2>&1
  make coverage-html >> $LOG 2>&1

I certainly expect that this would run the recovery test.  And today's log
file has this:

make -C recovery check
make[2]: Entering directory '/home/coverage/pgsrc/pgsql/src/test/recovery'
for extra in contrib/test_decoding; do make -C '../../..'/$extra 
DESTDIR='/home/coverage/pgsrc/pgsql'/tmp_install install 
>>'/home/coverage/pgsrc/pgsql'/tmp_install/log/install.log || exit; done
rm -rf '/home/coverage/pgsrc/pgsql/src/test/recovery'/tmp_check
/bin/mkdir -p '/home/coverage/pgsrc/pgsql/src/test/recovery'/tmp_check
cd . && TESTDIR='/home/coverage/pgsrc/pgsql/src/test/recovery' 
PATH="/home/coverage/pgsrc/pgsql/tmp_install/usr/local/pgsql/bin:$PATH" 
LD_LIBRARY_PATH="/home/coverage/pgsrc/pgsql/tmp_install/usr/local/pgsql/lib" 
PGPORT='65432' 
PG_REGRESS='/home/coverage/pgsrc/pgsql/src/test/recovery/../../../src/test/regress/pg_regress'
 /usr/bin/prove -I ../../../src/test/perl/ -I .  t/*.pl
t/001_stream_rep.pl .. ok
t/002_archiving.pl ... ok
t/003_recovery_targets.pl  ok
t/004_timeline_switch.pl . ok
t/005_replay_delay.pl  ok
t/006_logical_decoding.pl  ok
t/007_sync_rep.pl  ok
t/008_fsm_truncation.pl .. ok
t/009_twophase.pl  ok
t/010_logical_decoding_timelines.pl .. ok
t/011_crash_recovery.pl .. ok
t/012_subtransactions.pl . ok
t/013_crash_restart.pl ... ok
t/014_unlogged_reinit.pl . ok
t/015_promotion_pages.pl . ok
All tests successful.
Files=15, Tests=140, 100 wallclock secs ( 0.08 usr  0.02 sys + 18.70 cusr  8.57 
csys = 27.37 CPU)
Result: PASS
make[2]: Leaving directory '/home/coverage/pgsrc/pgsql/src/test/recovery'


Also:

$ cd src/backend/access/transam
$ ls -l xlog.*
-rw-r--r-- 1 coverage coverage 397975 Nov 19 06:01 xlog.c
-rw-r--r-- 1 coverage coverage  34236 Nov 20 09:20 xlog.gcda
-rw-r--r-- 1 coverage coverage 257868 Nov 20 09:02 xlog.gcno
-rw-r--r-- 1 coverage coverage 527576 Nov 20 09:02 xlog.o


Not sure what to make of this.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Tue, 20 Nov 2018 at 06:23, Vladimir Sitnikov 
wrote:

> Tom>Yes, we need either session open or reconnect it approach to find out
> Tom>the whether server is read-write or read-only.
>
> Just in case, pgjdbc has that feature for quite a while, and the behavior
> there is to keep the connection until it fails or application decides to
> close it.
>
> pgjdbc uses three parameters (since 2014):
> 1) targetServerType=(any | master | secondary | preferSecondary). Default
> is "any". When set to "master" it will look for "read-write" server. If set
> to "preferSecondary" it would search for "read-only" server first, then
> fall back to master, and so on.
> 2) loadBalanceHosts=(true | false). pgjdbc enables to load-balance across
> servers provided in the connection URL. When set to "false", pgjdbc tries
> connections in order, otherwise it shuffles the connections.
> 3) hostRecheckSeconds=int. pgjdbc caches "read/write" status of a
> host:port combination, so it don't re-check the status if multiple
> connections are created within hostRecheckSeconds timeframe.
>
> It is sad that essentially the same feature is re-implemented in core with
> different name/semantics.
> Does it make sense to align parameter names/semantics?
>

Looking at

https://www.postgresql.org/message-id/flat/1700970.cRWpxnom9y%40hammer.magicstack.net

Which Tom points out as being relevant to this discussion ISTM that this is
becoming a half baked "feature" that is being cobbled together instead of
being designed. Admittedly biased but I agree with Vladimir that libpq did
not implement the above feature using the same name and semantics. This
just serves to confuse the users.

Just my 2c worth


Dave Cramer

Re: Libpq support to connect to standby server as priority

[Vladimir Sitnikov]

Tom>Yes, we need either session open or reconnect it approach to find out
Tom>the whether server is read-write or read-only.

Just in case, pgjdbc has that feature for quite a while, and the behavior
there is to keep the connection until it fails or application decides to
close it.

pgjdbc uses three parameters (since 2014):
1) targetServerType=(any | master | secondary | preferSecondary). Default
is "any". When set to "master" it will look for "read-write" server. If set
to "preferSecondary" it would search for "read-only" server first, then
fall back to master, and so on.
2) loadBalanceHosts=(true | false). pgjdbc enables to load-balance across
servers provided in the connection URL. When set to "false", pgjdbc tries
connections in order, otherwise it shuffles the connections.
3) hostRecheckSeconds=int. pgjdbc caches "read/write" status of a host:port
combination, so it don't re-check the status if multiple connections are
created within hostRecheckSeconds timeframe.

It is sad that essentially the same feature is re-implemented in core with
different name/semantics.
Does it make sense to align parameter names/semantics?

Vladimir

Re: Shared Memory: How to use SYSV rather than MMAP ?

[Thomas Munro]

On Tue, Nov 20, 2018 at 11:11 PM REIX, Tony  wrote:
> On AIX, since with MMAP we have only 4K pages though we can have 64K pages 
> with SYSV, we'd like to experiment with SYSV rather than MMAP and measure the 
> impact to the performance.
>
> Looking at file: src/include/storage/dsm_impl.h , it seemed to me that 
> replacing the line:
>
> #define DEFAULT_DYNAMIC_SHARED_MEMORY_TYPE DSM_IMPL_POSIX
> by the line:
> #define DEFAULT_DYNAMIC_SHARED_MEMORY_TYPE DSM_IMPL_SYSV

Hi Tony,

SHOW dynamic_shared_memory_type to see which one it's actually using,
and set it in postgresql.conf to change it.

> However, when looking at details by means of procmap tool, it is unclear if 
> that worked or not.

These segments are short-lived ones used for parallel query.  I
haven't used AIX recently but I suspect procmap -X will show them as
different types and show the page size, but you'd have to check that
while it's actually running a parallel query.  For example, a large
parallel hash join that runs for a while would do it, and in theory
you might be able to see a small performance improvement for larger
page sizes due to better TLB cache hit ratios.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2

[Masahiko Sawada]

On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada  wrote:
>
> On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada  wrote:
> >
> > On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada  
> > wrote:
> > >
> > > On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada  
> > > wrote:
> > > >
> > > > On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
> > > >  wrote:
> > > > >
> > > > > Hello.
> > > > >
> > > > > # It took a long time to come here..
> > > > >
> > > > > At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada 
> > > > >  wrote in 
> > > > > 
> > > > > > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada 
> > > > > >  wrote:
> > > > > ...
> > > > > > * Updated docs, added the new section "Distributed Transaction" at
> > > > > > Chapter 33 to explain the concept to users
> > > > > >
> > > > > > * Moved atomic commit codes into src/backend/access/fdwxact 
> > > > > > directory.
> > > > > >
> > > > > > * Some bug fixes.
> > > > > >
> > > > > > Please reivew them.
> > > > >
> > > > > I have some comments, with apologize in advance for possible
> > > > > duplicate or conflict with others' comments so far.
> > > >
> > > > Thank youf so much for reviewing this patch!
> > > >
> > > > >
> > > > > 0001:
> > > > >
> > > > > This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
> > > > > relation is modified. Isn't it needed when UNLOGGED tables are
> > > > > modified? It may be better that we have dedicated classification
> > > > > macro or function.
> > > >
> > > > I think even if we do atomic commit for modifying the an UNLOGGED
> > > > table and a remote table the data will get inconsistent if the local
> > > > server crashes. For example, if the local server crashes after
> > > > prepared the transaction on foreign server but before the local commit
> > > > and, we will lose the all data of the local UNLOGGED table whereas the
> > > > modification of remote table is rollbacked. In case of persistent
> > > > tables, the data consistency is left. So I think the keeping data
> > > > consistency between remote data and local unlogged table is difficult
> > > > and want to leave it as a restriction for now. Am I missing something?
> > > >
> > > > >
> > > > > The flag is handled in heapam.c. I suppose that it should be done
> > > > > in the upper layer considering coming pluggable storage.
> > > > > (X_F_ACCESSEDTEMPREL is set in heapam, but..)
> > > > >
> > > >
> > > > Yeah, or we can set the flag after heap_insert in ExecInsert.
> > > >
> > > > >
> > > > > 0002:
> > > > >
> > > > > The name FdwXactParticipantsForAC doesn't sound good for me. How
> > > > > about FdwXactAtomicCommitPartitcipants?
> > > >
> > > > +1, will fix it.
> > > >
> > > > >
> > > > > Well, as the file comment of fdwxact.c,
> > > > > FdwXactRegisterTransaction is called from FDW driver and
> > > > > F_X_MarkForeignTransactionModified is called from executor. I
> > > > > think that we should clarify who is responsible to the whole
> > > > > sequence. Since the state of local tables affects, I suppose
> > > > > executor is that. Couldn't we do the whole thing within executor
> > > > > side?  I'm not sure but I feel that
> > > > > F_X_RegisterForeignTransaction can be a part of
> > > > > F_X_MarkForeignTransactionModified.  The callers of
> > > > > MarkForeignTransactionModified can find whether the table is
> > > > > involved in 2pc by IsTwoPhaseCommitEnabled interface.
> > > >
> > > > Indeed. We can register foreign servers by executor while FDWs don't
> > > > need to register anything. I will remove the registration function so
> > > > that FDW developers don't need to call the register function but only
> > > > need to provide atomic commit APIs.
> > > >
> > > > >
> > > > >
> > > > > >   if (foreign_twophase_commit == true &&
> > > > > >   ((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
> > > > > >   ereport(ERROR,
> > > > > >   
> > > > > > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > > > >errmsg("cannot COMMIT a distributed 
> > > > > > transaction that has operated on foreign server that doesn't 
> > > > > > support atomic commit")));
> > > > >
> > > > > The error is emitted when a the GUC is turned off in the
> > > > > trasaction where MarkTransactionModify'ed. I think that the
> > > > > number of the variables' possible states should be reduced for
> > > > > simplicity. For example in the case, once foreign_twopase_commit
> > > > > is checked in a transaction, subsequent changes in the
> > > > > transaction should be ignored during the transaction.
> > > > >
> > > >
> > > > I might have not gotten your comment correctly but since the
> > > > foreign_twophase_commit is a PGC_USERSET parameter I think we need to
> > > > check it at commit time. Also we need to keep participant servers even
> > > > when foreign_twophase_commit is off if both max_prepared_foreign_xacts
> > > > and max_foreign_xact_resolvers are > 0.
> > > >
> > > > I will post the updated patch 

Re: [PATCH] Opclass parameters

[Nikolay Shaplov]

В письме от 15 ноября 2018 18:26:43 пользователь Nikita Glukhov написал:

> Attached 2nd version of the patches. Nothing has changed since March,
> this is just a rebased version.
> 
> CREATE INDEX syntax and parameters storage method still need discussion.
I've played around a bit with you patch and come to some conclusions, I'd like 
to share. They are almost same as those before, but now there are more 
details.

Again some issues about storing opclass options in pg_inedx:

1. Having both indoption and indoptions column in pg_index will make someone's 
brain explode for sure. If not, it will bring troubles when people start 
confusing them.

2. Now I found out how do you store option values for each opclass: text[] of 
indoptions in pg_index is not the same as text[] in 
reloptions in pg_catalog (and it brings more confusion). In reloption each 
member of the array is a single option. 

reloptions  | {fillfactor=90,autovacuum_enabled=false}

In indoptions, is a whole string of options for one of the indexed attributes, 
each array item has all options for one indexed attribute. And this string 
needs furthermore parsing, that differs from reloption parsing.

indoptions | {"{numranges=150}","{numranges=160}"}


This brings us to the following issues:

2a. pg_index stores properties of index in general. Properties of each indexed 
attributes is stored in pg_attribute table. If we follow this philosophy
it is wrong to add any kind of per-attribute array values into pg_index. These 
values should be added to pg_attribute one per each pg_attribute entry.

2b. Since you've chosen method of storage that differs from one that is used 
in reloptions, that will lead to two verstions of code that processes the 
attributes. And from now on, if we accept this, we should support both of them 
and keep them in sync. (I see that you tried to reuse as much code as 
possible, but still you added some more that duplicate current reloptions 
functionality.)

I know that relotions code is not really suitable for reusing. This was the 
reason why I started solving oplcass option task with rewriting reloptions 
code,to make it 100% reusable for any kind of options. So I would offer you 
again to join me as a reviewer of that code. This will make opclass code more 
simple and more sensible, if my option code is used... 

3. Speaking of sensible code

Datum
g_int_options(PG_FUNCTION_ARGS)
{
   Datum   raw_options = PG_GETARG_DATUM(0);
   boolvalidate = PG_GETARG_BOOL(1);
   relopt_int  siglen =
   { {"numranges", "number of ranges for compression", 0, 0, 9, 
RELOPT_TYPE_INT },
   G_INT_NUMRANGES_DEFAULT, 1, G_INT_NUMRANGES_MAX };
   relopt_gen *optgen[] = {  };
   int offsets[] = { offsetof(IntArrayOptions, num_ranges) };
   IntArrayOptions *options =
   parseAndFillLocalRelOptions(raw_options, optgen, offsets, 1,
   sizeof(IntArrayOptions), validate);

   PG_RETURN_POINTER(options);
}

It seems to be not a very nice hack.
What would you do if you would like to have both int, real and boolean options 
for one opclass? I do not know how to implement it using this code.
We have only int opclass options for now, but more will come and we should be 
ready for it.

4. Now getting back to not adding opclass options wherever we can, just 
because we can:

4a. For inrarray there were no opclass options tests added. I am sure there 
should be one, at least just to make sure it still does not segfault when you 
try to set one. And in some cases more tests can be needed. To add and review 
them one should be familiar with this opclass internals. So it is good when 
different people do it for different opclasses

4b. When you add opclass options instead of hardcoded values, it comes to 
setting minimum and maximum value. Why do you choose 1000 as maximum 
for num_ranges in gist_int_ops in intarray? Why 1 as minimum? All these 
decisions needs careful considerations and can't be done for bunch of 
opclasses just in one review. 

4c. Patch usually take a long path from prototype to final commit. Do you 
really want to update all these opclasses code each time when some changes 
in the main opclass option code is made? ;-)

So I would suggest to work only with intarray and add other opclasses later.

5. You've been asking about SQL grammar

> CREATE INDEX idx ON tab USING am (
>{expr {opclass | DEFAULT} ({name=value} [,...])} [,...]
> );

As for me I do not really care about it. For me all the solutions is 
acceptable. But looking at is i came to one notion:

I've never seen before DEFAULT keyword to be used in this way. There is logic 
in such usage, but I did not remember any practical usage case.
If there are such usages (I can easily missed it) or if it is somehow 
recommended in SQL standard -- let it be. But if none above, I would suggest 
to use WITH keyword instead. As it is already used for reloptions. As far as I 
remember in my prototype I used 

Re: mysql_fdw crash

[Pavel Stehule]

Hi

út 20. 11. 2018 v 11:09 odesílatel <066ce...@free.fr> napsal:

> Hi,
>
> I do have a reproductible crash with mysql_fdw when executing a plpgsql
> function. I'm running pg 11.1 with current mysql_fdw, but I had the same
> crash with the pg 9.6 and mysql_fdw provided with ubuntu packages.
>
> From psql side :
>
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>
>
> In syslog :
>
> Nov 20 10:52:58 sact2Dev kernel: [322982.294765] postgres[10364]: segfault
> at 0 ip 7fc8ab7b5350 sp 7ffc4312a4f0 error 6 in
> mysql_fdw.so[7fc8ab7ac000+d000]
>
> I've located the crash cause on the line :
>
> *param_types[i] = exprType(param_expr);
>
> ( file mysql_fdw.c ; function prepare_query_params() ; in the forEach()
> loop)
>
> I've recompiled the fdw with a -g option. Could you please tell me (or
> point me a documentation) how to have a core dump from the segfaulted lib ;
> so that I can open it in a debugger to inspect variable contents ?
>
>
In this case the most simply technique is attaching to live postgresql
session by gdb

I use a small script that run gdb and attach first postgresql session

#!/bin/bash

PID=`ps ax|grep postgres | grep 'postgres: .*idle$' | awk '{print $1}'`

gdb /usr/local/pgsql/bin/postmaster -p $PID

When gdb will be active, then use command c, and then run query in session.
gdb should to catch segfault.

Regards

Pavel




> Or any advice ?
>
> Thank you.
>
>

Re: Psql patch to show access methods info

[Alvaro Herrera]

On 2018-Nov-20, s.cherkas...@postgrespro.ru wrote:

> Yes, I am available to finish this patch.
> I’m sorry that I hadn’t updated patch for new commitfest and I grateful to
> you for doing it and fixing some issues.
> I would like to clarify which commands lack the output of the schema names?
> Because I tried to display them for all objects that have a schema.

I think Michael is referring to the queries used to obtain the data.
For example "FROM pg_class c" is bogus -- it must be "FROM
pg_catalog.pg_class c".

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Psql patch to show access methods info

[s . cherkashin]

Yes, I am available to finish this patch.
I’m sorry that I hadn’t updated patch for new commitfest and I grateful 
to you for doing it and fixing some issues.
I would like to clarify which commands lack the output of the schema 
names? Because I tried to display them for all objects that have a 
schema.


Best regards,
Sergej Cherkashin.

On 2018-11-19 05:38, Alvaro Herrera wrote:

On 2018-Nov-19, Michael Paquier wrote:


On Sat, Nov 17, 2018 at 11:20:50PM -0300, Alvaro Herrera wrote:
> Here's a rebased version, fixing the rejects, pgindenting, and fixing
> some "git show --check" whitespace issues.  Haven't reviewed any further
> than that.

Schema qualifications are missing in many places, and they are added
sometimes.  The character limit in documentation paragraph could be 
more

respected as well.


Sergey, are you available to fix these issues?  Nikita?

Thanks

Shared Memory: How to use SYSV rather than MMAP ?

[REIX, Tony]

Hi,


On AIX, since with MMAP we have only 4K pages though we can have 64K pages with 
SYSV, we'd like to experiment with SYSV rather than MMAP and measure the impact 
to the performance.


Looking at file: src/include/storage/dsm_impl.h , it seemed to me that 
replacing the line:

#define DEFAULT_DYNAMIC_SHARED_MEMORY_TYPE DSM_IMPL_POSIX
by the line:
#define DEFAULT_DYNAMIC_SHARED_MEMORY_TYPE DSM_IMPL_SYSV

was the right thing to do. Plus some changes like:

export LDR_CNTRL=SHMPSIZE=64K

ldedit -btextpsize=64k -bdatapsize=64k -bstackpsize=64k ./postgres


However, when looking at details by means of procmap tool, it is unclear if 
that worked or not.

Maybe I was lost by the variables:

 HAVE_SHM_OPEN  .  USE_DSM_POSIX   .   USE_DSM_SYSV   .   USE_DSM_MMAP

which are all defined.



So, what should I do in order to use SYSV rather than MMAP for the Shared 
Memory ?

(PostgreSQL v11.1)


Thanks/Regards,


Cordialement,

Tony Reix

tony.r...@atos.net

ATOS / Bull SAS
ATOS Expert
IBM Coop Architect & Technical Leader
Office : +33 (0) 4 76 29 72 67
1 rue de Provence - 38432 Échirolles - France
www.atos.net

mysql_fdw crash

[066ce286]

Hi,

I do have a reproductible crash with mysql_fdw when executing a plpgsql 
function. I'm running pg 11.1 with current mysql_fdw, but I had the same crash 
with the pg 9.6 and mysql_fdw provided with ubuntu packages.

>From psql side :

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

In syslog :

Nov 20 10:52:58 sact2Dev kernel: [322982.294765] postgres[10364]: segfault at 0 
ip 7fc8ab7b5350 sp 7ffc4312a4f0 error 6 in 
mysql_fdw.so[7fc8ab7ac000+d000]

I've located the crash cause on the line :

*param_types[i] = exprType(param_expr);

( file mysql_fdw.c ; function prepare_query_params() ; in the forEach() loop)

I've recompiled the fdw with a -g option. Could you please tell me (or point me 
a documentation) how to have a core dump from the segfaulted lib ; so that I 
can open it in a debugger to inspect variable contents ?

Or any advice ?

Thank you.

Re: Sync ECPG scanner with core

[John Naylor]

I wrote:

> On 11/14/18, Tom Lane  wrote:
>> Alvaro Herrera  writes:
>>> Maybe time to compile the ecpg test cases during "make check-world"?
>>
>> I'm dubious that the lexer is a significant part of that, though I could
>> be wrong ...
>
> If it were, it'd be much easier to try a Flex flag other than the
> default, most compact representation, something like -Cfe. That'd be a
> prerequisite for no-backtracking to matter anyway. That's easy enough
> to test, so I'll volunteer to do that sometime.

The preproc phase of make check in the ecpg dir only takes 150ms.
Compiling and linking the test executables takes another 2s, and
running the tests takes another 5s on my machine. Even without setting
up and tearing down the temp instance, preproc is trivial.

-John Naylor

Re: [RFC] Removing "magic" oids

[Andres Freund]

On 2018-11-14 21:02:41 -0800, Andres Freund wrote:
> Hi,
> 
> On 2018-11-15 04:57:28 +, Noah Misch wrote:
> > On Wed, Nov 14, 2018 at 12:01:52AM -0800, Andres Freund wrote:
> > > - one pgbench test tested concurrent insertions into a table with
> > >   oids, as some sort of stress test for lwlocks and spinlocks. I *think*
> > >   this doesn't really have to be a system oid column, and this was just
> > >   because that's how we triggered a bug on some machine. Noah, do I get
> > >   this right?
> > 
> > The point of the test is to exercise OidGenLock by issuing many parallel
> > GetNewOidWithIndex() and verifying absence of duplicates.  There's nothing
> > special about OidGenLock, but it is important to use an operation that 
> > takes a
> > particular LWLock many times, quickly.  If the test query spends too much 
> > time
> > on things other than taking locks, it will catch locking races too rarely.
> 
> Sequences ought to do that, too. And if it's borked, we'd hopefully see
> unique violations. But it's definitely not a 1:1 replacement.
> 
> 
> > >  pgbench(
> > >   '--no-vacuum --client=5 --protocol=prepared --transactions=25',
> > >   0,
> > >   [qr{processed: 125/125}],
> > >   [qr{^$}],
> > > - 'concurrency OID generation',
> > > + 'concurrent insert generation',
> > >   {
> > > - '001_pgbench_concurrent_oid_generation' =>
> > > -   'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);'
> > > + '001_pgbench_concurrent_insert' =>
> > > +   'INSERT INTO insert_tbl SELECT FROM generate_series(1,1000);'
> > 
> > The code for sequences is quite different, so this may or may not be an
> > effective replacement.  To study that, you could remove a few barriers from
> > lwlock.c, measure how many iterations today's test needs to catch the
> > mutation, and then measure the same for this proposal.
> 
> Unfortunately it's really hard to hit barrier issues on x86. I think
> that's the only arch I currently have access to, but it's possible I
> have access to some ppc too.  If you have a better idea for a
> replacement test, I'd be all ears.

I've tested this on ppc.  Neither the old version nor the new version
stress test spinlocks sufficiently to error out with weakened spinlocks
(not that surprising, there are no spinlocks in any hot path of either
workload). Both versions very reliably trigger on weakened lwlocks. So I
think we're comparatively good on that front.

Greetings,

Andres Freund

Re: Patch to avoid SIGQUIT accident

[Christoph Berg]

Re: Tom Lane 2018-10-22 <80020.1540164...@sss.pgh.pa.us>
> * SIGQUIT is a fairly well-known way to get out of an application when all
> else fails.  People who aren't familiar with psql's exit commands might
> find it pretty unfriendly of us to block this off.

Fwiw, pgadmin4 is one of those. ^C doesn't do anything, but ^\ works.
Please don't "fix" that problem.

Christoph

Re: pg_stat_ssl additions

[Kyotaro HORIGUCHI]

Hello.

At Thu, 18 Oct 2018 00:05:15 +0200, Peter Eisentraut 
 wrote in 
<398754d8-6bb5-c5cf-e7b8-22e5f0983...@2ndquadrant.com>
> During discussions of alternative SSL implementations, contrib/sslinfo
> is usually mentioned as something that something needs to be done about.
>  I've looked into adapting some functionality from sslinfo into the
> pg_stat_ssl view.  These two facilities have a lot of overlap but seem
> mostly oblivious to each other.
> 
> The attached patch series
> 
> - Adds a documentation link from sslinfo to pg_stat_ssl.
> 
> - Adds tests under src/test/ssl/ for the pg_stat_ssl view.

With this change the feature mapping between the two is as
follows.

  ssl_is_used() : .ssl
  ssl_version() : .version
  ssl_cipher()  : .cipher
  (none): .bits
  (none): .compression
  ssl_client_cert_present() : .clientdn IS NOT NULL
  ssl_client_serial()   : .clientserial
  ssl_client_dn()   : .clientdn
  ssl_issuer_dn()   : .issuerdn
  ssl_client_dn_field() : (none)
  ssl_issuer_field(): (none)
  ssl_extension_info()  : (none)

# I couldn't create x509 v3 certs for uncertain reasons..

Comparing the two codes in pgstatfuncs.c and sslinfo.c, It seems
that ssl_client_serial() can be largely simplified using
be_tls_get_peer_serial(). X509_NAME_to_text() can be simplified
using X509_NAME_to_cstring(). But this would be another patch.

By the way, though it is not by this patch, X509_NAME_to_cstring
doesn't handle NID_undef from OBJ_obj2nid() and NULL from
OBJ_nid2ln(). The latter case feeds NULL to BIO_printf() . I'm
not sure it can actually happen.

I don't look through all the similar points but it might be
better fix it.

> - Changes pg_stat_ssl.clientdn to be null if there is no client
> certificate (as documented, but not implemented). (bug fix)

This reveals DN, serial and issuer DN of the cert to
non-superusres. pg_stat_activity hides at least
client_addr. Aren't they needed to be hidden? (This will change
the existing behavior.)

> - Adds new fields to pg_stat_ssl: issuerdn and clientserial.  These
> allow uniquely identifying the client certificate.  AFAICT, these are
> the most interesting pieces of information provided by sslinfo but not
> in pg_stat_ssl.  (I don't like the underscore-free naming of these
> fields, but it matches the existing "clientdn".)

clientdn, clientserial, issuerdn are the fields about client
certificates. Only the last one omits prefixing "client". But
"clientissuerdn" seems somewhat rotten... Counldn't we rename
clientdn to client_dn?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

incorrect xlog.c coverage report

[Peter Eisentraut]

I noticed some strangeness in the test coverage reporting.  For example,
in
https://coverage.postgresql.org/src/backend/access/transam/xlog.c.gcov.html
in the function readRecoveryCommandFile(), most of the branches parsing
the individual recovery options (recovery_target_xid,
recovery_target_time, etc.) are shown as never hit, even though there
are explicit tests for this in
src/test/recovery/t/003_recovery_targets.pl.  I tried this locally and
also with -O0 just in case, but I get the same results.  Any ideas?

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

Re: ToDo: show size of partitioned table

[Amit Langote]

On 2018/11/20 16:50, Michael Paquier wrote:
> Testing the feature, \dP shows all partitioned relations, still does not
> show the relationship when multiple levels are used.  Could it make
> sense to also show the direct parent of a partitioned table when
> verbose mode is used?

Yeah.  I think it would make sense for \dP output to have an additional
column(s) for partitioning-specific information if we're building a
special command for partitioning anyway.

Thanks,
Amit

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

I wrote:

> On 11/19/18, Amit Kapila  wrote:
> [ abortive states ]
>> I think it might come from any other place between when you set it and
>> before it got cleared (like any intermediate buffer and pin related
>> API's).
>
> Okay, I will look into that.

LockBuffer(), visibilitymap_pin(), and GetVisibilityMapPins() don't
call errors at this level. I don't immediately see any additional good
places from which to clear the local map.

-John Naylor