Re: ERROR: tuple concurrently updated when modifying privileges
On Tue, May 14, 2019 at 08:08:05AM +0200, Chris Travers wrote: > Having thought about this a bit, I think the best solution would be to have > grant take out an access share lock to the tables granted. This would > prevent concurrent alter table operations from altering the schema > underneath the grant as well, and thus possibly cause other race conditions. > > Any thoughts? "tuple concurrently updated" is an error message which should never be user-facing, and unfortunately there are many scenarios where it can be triggered by playing with concurrent DDLs: https://postgr.es/m/20171228063004.gb6...@paquier.xyz If you have an idea of patch, could you write it? Having an isolation test case would be nice as well. -- Michael signature.asc Description: PGP signature
can we create index/constraints in different schema
Hi, This is working in Oracle but not in postgresql 'CREATE INDEX client.test_1_idx ON dbo.test_1 (name);' . Can we implement this by another way? Thanks Navneet
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 06:58:14PM +1200, Thomas Munro wrote: > On Tue, May 14, 2019 at 6:18 PM Kyotaro HORIGUCHI > wrote: > > I played with this a bit and found that "... (attr=[tab]" (no > > space between "r" and "=") complets with '='. Isn't it annoying? > > > > Only "UPDATE hoge SET a=[tab]" behaves the same way among > > existing completions. > > Hmm. True. Here's one way to fix that. Hmm... just got here. What happens around here? > > -- > Thomas Munro > https://enterprisedb.com
Re: can we create index/constraints in different schema
On Tue, May 14, 2019 at 03:41:37AM -0400, navneet nikku wrote: >This is working in Oracle but not in postgresql 'CREATE INDEX > client.test_1_idx > ON dbo.test_1 (name);' . Can we implement this by another way? No, it is not possible to define a schema with CREATE INDEX, and an index gets located in the same schema as its depending table. -- Michael signature.asc Description: PGP signature
Re: can we create index/constraints in different schema
Ok, thanks for the clarification. Regards Navneet On Tue, May 14, 2019 at 4:33 AM Michael Paquier wrote: > On Tue, May 14, 2019 at 03:41:37AM -0400, navneet nikku wrote: > >This is working in Oracle but not in postgresql 'CREATE INDEX > > client.test_1_idx > > ON dbo.test_1 (name);' . Can we implement this by another way? > > No, it is not possible to define a schema with CREATE INDEX, and an > index gets located in the same schema as its depending table. > -- > Michael >
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 8:32 PM Edgy Hacker wrote: > Hmm... just got here. Welcome. > What happens around here? Please see https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F . -- Thomas Munro https://enterprisedb.com
Re: pg_stat_database update stats_reset only by pg_stat_reset
it reset statistics for a single table and update the column stats_reset of
pg_stat_database.
but i think that stats_reset shoud be database-level statistics, a single
table should not update the column stats_reset.
i am monitor the xact_commit every 5 minutes, when stats_reset is reset but
ohter columns is not reset, i can't decide
if i will recount the xact_commit, because pg_stat_reset make all column to
zero. pg_stat_reset_single_table_counters
only reset the column stats_reset.
张连壮 于2019年5月13日周一 下午3:30写道：
> pg_stat_reset_single_table_counters/pg_stat_reset_single_function_counters
> only update pg_stat_database column stats_reset.
> stat_reset shuld update when all the column is reset.
>
> sample:
> drop database if exists lzzhang_db;
> create database lzzhang_db;
> \c lzzhang_db
>
> create table lzzhang_tab(id int);
> insert into lzzhang_tab values(1);
> insert into lzzhang_tab values(1);
>
> select tup_fetched, stats_reset from pg_stat_database where
> datname='lzzhang_db';
> select pg_sleep(1);
>
> select pg_stat_reset_single_table_counters('lzzhang_tab'::regclass::oid);
> select tup_fetched, stats_reset from pg_stat_database where
> datname='lzzhang_db';
>
> result:
> tup_fetched | stats_reset
> -+---
> 514 | 2019-05-12 03:22:55.702753+08
> (1 row)
> tup_fetched | stats_reset
> -+---
> 710 | 2019-05-12 03:22:56.729336+08
> (1 row)
> tup_fetched is not reset but stats_reset is reset.
>
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 09:01:27PM +1200, Thomas Munro wrote: > On Tue, May 14, 2019 at 8:32 PM Edgy Hacker wrote: > > Hmm... just got here. > > Welcome. Thanks. > > > What happens around here? > > Please see https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F > . Not exactly a prospective developer but if it ever comes it... > > -- > Thomas Munro > https://enterprisedb.com
Re: ERROR: tuple concurrently updated when modifying privileges
On Tue, May 14, 2019 at 9:11 AM Michael Paquier wrote: > On Tue, May 14, 2019 at 08:08:05AM +0200, Chris Travers wrote: > > Having thought about this a bit, I think the best solution would be to > have > > grant take out an access share lock to the tables granted. This would > > prevent concurrent alter table operations from altering the schema > > underneath the grant as well, and thus possibly cause other race > conditions. > > > > Any thoughts? > > "tuple concurrently updated" is an error message which should never be > user-facing, and unfortunately there are many scenarios where it can > be triggered by playing with concurrent DDLs: > https://postgr.es/m/20171228063004.gb6...@paquier.xyz > > If you have an idea of patch, could you write it? Having an isolation > test case would be nice as well. > I will give Nick a chance to do the patch if he wants it (I have reached out). Otherwise sure. I did notice one more particularly exotic corner case that is not resolved by this proposed fix. If you have two transactions with try to grant onto the same pg entity (table etc) *both* will typically fail on the same error. I am not sure that is a bad thing because I am not sure how concurrent grants are supposed to work with MVCC but I think that would require a fundamentally different approach. > -- > Michael > -- Best Regards, Chris Travers Head of Database Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com Saarbrücker Straße 37a, 10405 Berlin
Re: PG 12 draft release notes
On Sat, May 11, 2019, at 22:33, Bruce Momjian wrote: > http://momjian.us/pgsql_docs/release-12.html There is a typo in E.1.3.1.1.: > Expressions are evaluated at table partitioned table creation time. First "table" seems to be excessive. Regards, Nick.
Read-only access to temp tables for 2PC transactions
Hi, That is an attempt number N+1 to relax checks for a temporary table access in a transaction that is going to be prepared. One of the problems regarding the use of temporary tables in prepared transactions is that such transaction will hold locks for a temporary table after being prepared. That locks will prevent the backend from exiting since it will fail to acquire lock needed to delete temp table during exit. Also, re-acquiring such lock after server restart seems like an ill-defined operation. I tried to allow prepared transactions that opened a temporary relation only in AccessShare mode and then neither transfer this lock to a dummy PGPROC nor include it in a 'prepare' record. Such prepared transaction will not prevent the backend from exiting and can be committed from other backend or after a restart. However, that modification allows new DDL-related serialization anomaly: it will be possible to prepare transaction which read table A; then drop A; then commit the transaction. I not sure whether that is worse than not being able to access temp relations or not. On the other hand, it is possible to drop AccessShare locks only for temporary relation and don't change behavior for an ordinary table (in the attached patch this is done for all tables). Also, I slightly modified ON COMMIT DELETE code path. Right now all ON COMMIT DELETE temp tables are linked in a static list and if transaction accessed any temp table in any mode then during commit all tables from that list will be truncated. For a given patch that means that even if a transaction only did read from a temp table it anyway can access other temp tables with high lock mode during commit. I've added hashtable that tracks higher-than-AccessShare action with a temp table during current transaction, so during commit, only tables from that hash will be truncated. That way ON COMMIT DELETE tables in the backend will not prevent read-only access to some other table in a given backend. Any thoughts? -- Stas Kelvich Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 2PC-ro-temprels.patch Description: Binary data
Re: Failure in contrib test _int on loach
On 08/05/2019 01:31, Heikki Linnakangas wrote: On 02/05/2019 10:37, Heikki Linnakangas wrote: On 29/04/2019 16:16, Anastasia Lubennikova wrote: In previous emails, I have sent two patches with test and bugfix (see attached). After Heikki shared his concerns, I've rechecked the algorithm and haven't found any potential error. So, if other hackers are agreed with my reasoning, the suggested fix is sufficient and can be committed. I still believe there is a problem with grandparent splits with this. I'll try to construct a test case later this week, unless you manage to create one before that. Here you go. If you apply the two patches from https://www.postgresql.org/message-id/5d48ce28-34cf-9b03-5d42-dbd5457926bf%40postgrespro.ru, and run the attached script, it will print out something like this: postgres=# \i grandparent.sql DROP TABLE CREATE TABLE INSERT 0 15 CREATE INDEX psql:grandparent.sql:27: NOTICE: working on 1 psql:grandparent.sql:27: NOTICE: working on 2 psql:grandparent.sql:27: NOTICE: working on 3 psql:grandparent.sql:27: NOTICE: working on 4 psql:grandparent.sql:27: NOTICE: working on 5 psql:grandparent.sql:27: NOTICE: working on 6 psql:grandparent.sql:27: NOTICE: working on 7 psql:grandparent.sql:27: NOTICE: working on 8 psql:grandparent.sql:27: NOTICE: working on 9 psql:grandparent.sql:27: NOTICE: working on 10 psql:grandparent.sql:27: NOTICE: working on 11 psql:grandparent.sql:27: NOTICE: failed for 114034 psql:grandparent.sql:27: NOTICE: working on 12 DO That "failed for 114034" should not happen. Fortunately, that's not too hard to fix. We just need to arrange things so that the "retry_from_parent" flag also gets set for the grandparent, when the grandparent is split. Like in the attached patch. I hear no objections, so pushed that. But if you have a chance to review this later, just to double-check, I'd still appreciate that. - Heikki
Re: Tab completion for CREATE TYPE
At Tue, 14 May 2019 18:58:14 +1200, Thomas Munro wrote
in
> On Tue, May 14, 2019 at 6:18 PM Kyotaro HORIGUCHI
> wrote:
> > I played with this a bit and found that "... (attr=[tab]" (no
> > space between "r" and "=") complets with '='. Isn't it annoying?
> >
> > Only "UPDATE hoge SET a=[tab]" behaves the same way among
> > existing completions.
>
> Hmm. True. Here's one way to fix that.
Thanks. That's what was in my mind.
Some definition item names are induced from some current states
(e.g. "CREATE TYPE name AS RANGE (" => "SUBTYPE = ") but I think
it's too much.
COLLATE is not suggested with possible collations but I think
suggesting it is not so useful.
PASSEDBYVALUE is suggested with '=', which is different from
documented syntax but I don't think that's not such a problem for
those who spell this command out.
# By the way, collatable and preferred are boolean which behaves
# the same way with passedbyvalue. Is there any intention in the
# difference in the documentation?
The completion lists contain all possible words correctly (I
think "analyse" is an implicit synonym.).
As the result, I find it perfect.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Table AM callback table_complete_speculative()'s succeeded argument is reversed
The 'succeeded' argument seems backwards here:
static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
uint32
spekToken, bool succeeded)
{
boolshouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
/* adjust the tuple's state accordingly */
if (!succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);
if (shouldFree)
pfree(tuple);
}
According to the comments, if "succeeded = true", the insertion is
completed, and otherwise it's killed. It works, because the only caller
is also passing the argument wrong.
Barring objections, I'll push the attached patch to fix that.
- Heikki
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bc47856ad5..00505ec3f4 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -282,7 +282,7 @@ heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
/* adjust the tuple's state accordingly */
- if (!succeeded)
+ if (succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 444c0c0574..d545bbce8a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -556,7 +556,7 @@ ExecInsert(ModifyTableState *mtstate,
/* adjust the tuple's state accordingly */
table_complete_speculative(resultRelationDesc, slot,
- specToken, specConflict);
+ specToken, !specConflict);
/*
* Wake up anyone waiting for our decision. They will re-check
RE: Copy data to DSA area
Hi,
>>From: Thomas Munro [mailto:thomas.mu...@gmail.com]
>>Subject: Re: Copy data to DSA area
>>
>>On Wed, May 8, 2019 at 5:29 PM Ideriha, Takeshi
>>
>>wrote:
>>> >From: Ideriha, Takeshi [mailto:ideriha.take...@jp.fujitsu.com]
>>> >Sent: Friday, April 26, 2019 11:50 PM Well, after developing PoC, I
>>> >realized that this PoC doesn't solve the local process is crashed
>>> >before the context becomes shared because local process keeps track
>>> >of pointer to chunks.
>>> >Maybe all of you have already noticed and pointed out this case :)
>>> >So it needs another work but this poc is a good step for me to advance
>>> >more.
>>>
>>> I think the point to prevent memory leak is allocating memory and
>>> storing its address into a structure at the same time. This structure
>>> should be trackable from other process.
>>
>>I'm not sure that it's necessarily wrong to keep tracking information in
>>private
>memory.
>>If any backend crashes, the postmaster will terminate all backends and
>>reinitialise everything anyway, because shared memory might be corrupted.
>
>I'm going to put keep tracking information in private
>memory and send a patch.
I updated a PoC patch.
This has memory tracking buffer in local process. The old version also has this
system but I refactored the code. Memory leak while allocating memory seems to
be solved thanks to memory tracking buffer.
What I haven't addressed is memory leak while freeing objects. In current
sequence a cache (e.g. relcache) is freed after removed from its hash table.
If cache and hash table gets shared, memory leak is likely to happen
between removal from hash table and free. We lose track of cache objects
if error happens after cache is unlinked from the hash table. And also a cache
consists of graph structure. So we also take care of freeing cache partially.
Maybe we need to remember pointers of objects before unlink from the hash.
Also, we need to free them all at once after we can make sure that all the
pointers are registered to local buffer. Followings are some idea to implement
this:
- change the order of removal from hash table and deletion
- pfree in shared memory context doesn't dsa_free but just add pointer to
the local buffer.
- remove link from hash table after all pfree() is done
- then, call a function, which does actual dsa_free taking a look at the local
Buffer
But I'm not sure this solution is good one. Do you have any thoughts?
Regards,
Takeshi Ideriha
/*
* Another PoC of MemoryContext for DSA based on Thomus Munro's work
*
* https://www.postgresql.org/message-id/
* CAEepm%3D22H0TDCAHWh%3DYWmSV%2BX%2BbTtcTNg8RpP%3DeaCjWJU_d-9A
* %40mail.gmail.com
*
*/
#include "postgres.h"
#include "fmgr.h"
#include "lib/ilist.h"
#include "miscadmin.h"
#include "nodes/pg_list.h"
#include "nodes/memnodes.h"
#include "storage/ipc.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "utils/dsa.h"
#include "utils/memutils.h"
#define MY_AREA_SIZE (1024 * 1024)
#define NUM_CHUNKS 64 /* an arbitrary number */
PG_MODULE_MAGIC;
void _PG_init(void);
PG_FUNCTION_INFO_V1(hoge);
PG_FUNCTION_INFO_V1(hoge_list);
PG_FUNCTION_INFO_V1(hoge_list_error);
#define isLocalContext(c) (c->alloc_buffer != NULL)
#define isBufferFull(buf) (buf->tail == NUM_CHUNKS)
/* Support code to make a dsa_area into a MemoryContext. */
typedef struct dsa_temp_buffer dsa_temp_buffer;
static void hoge_shmem_startup_hook(void);
static MemoryContext make_hoge_memory_context(dsa_area *area, void *base, bool
isLocal);
static void push_new_buffer(dsa_temp_buffer *buffer);
static shmem_startup_hook_type prev_shmem_startup_hook;
static void *my_raw_memory;
static dsa_area *my_area;
static MemoryContext my_shared_dsa_context;
static MemoryContext my_local_dsa_context;
static List **my_list;
void AddToDsaSharedContext(MemoryContext local, MemoryContext shared);
MemoryContext CreateDsaLocalContext(MemoryContext shared);
Size ShmDsaContextSize(void);
void
_PG_init(void)
{
/* This only works if preloaded by the postmaster. */
if (!process_shared_preload_libraries_in_progress)
return;
/* Request a chunk of traditional shared memory. */
RequestAddinShmemSpace(MY_AREA_SIZE);
/* Register our hook for phase II of initialization. */
prev_shmem_startup_hook = shmem_startup_hook;
shmem_startup_hook = hoge_shmem_startup_hook;
}
static void
hoge_shmem_startup_hook(void)
{
MemoryContext old_context;
boolfound;
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
old_context = MemoryContextSwitchTo(TopMemoryContext);
LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
/* Allocate, or look up, a chunk of raw fixed-address shared memory. */
my_raw_memory = ShmemInitStruct("hoge", MY_AREA_SIZE, &found);
if (!found)
{
/*
* Create a new DSA area, and clamp its size so it can't
Re: How to install login_hook in Postgres 10.5
On 5/13/19 8:32 PM, Michael Paquier wrote: > On Mon, May 13, 2019 at 01:06:10PM -0700, legrand legrand wrote: >> that finished commited >> "pgsql: Add hooks for session start and session end" >> https://www.postgresql.org/message-id/flat/575d6fa2-78d0-4456-8600-302fc35b2591%40dunslane.net#0819e315c6e44c49a36c69080cab644d >> >> but was finally rollbacked because it didn't pass installcheck test >> ... >> >> Maybe you are able to patch your pg installation, >> it would be a solution of choice (there is a nice exemple >> of extension included) > > You will need to patch Postgres to add this hook, and you could > basically reuse the patch which has been committed once. I don't > think that it would be that much amount of work to get it working > correctly on the test side to be honest, so we may be able to get > something into v13 at this stage. This is mainly a matter of > resources though, and of folks willing to actually push for it. I am interested in this, so if Andrew wants to create a buildfarm module I will either add it to rhinoceros or stand up another buildfarm animal for it. I am also happy to help push it for v13. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: PG 12 draft release notes
On Tue, May 14, 2019 at 11:53:23AM +0200, nickb wrote: > On Sat, May 11, 2019, at 22:33, Bruce Momjian wrote: > > http://momjian.us/pgsql_docs/release-12.html > > There is a typo in E.1.3.1.1.: > > Expressions are evaluated at table partitioned table creation time. > First "table" seems to be excessive. Yep, fixed, thanks. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Table AM callback table_complete_speculative()'s succeeded argument is reversed
Hi,
On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas wrote:
>The 'succeeded' argument seems backwards here:
>
>> static void
>> heapam_tuple_complete_speculative(Relation relation, TupleTableSlot
>*slot,
>>uint32
>> spekToken, bool succeeded)
>> {
>> boolshouldFree = true;
>> HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
>>
>> /* adjust the tuple's state accordingly */
>> if (!succeeded)
>> heap_finish_speculative(relation, &slot->tts_tid);
>> else
>> heap_abort_speculative(relation, &slot->tts_tid);
>>
>> if (shouldFree)
>> pfree(tuple);
>> }
>
>According to the comments, if "succeeded = true", the insertion is
>completed, and otherwise it's killed. It works, because the only caller
>
>is also passing the argument wrong.
Thanks for finding.
>Barring objections, I'll push the attached patch to fix that.
Please hold off - your colleagues found this before, and I worked on getting
test coverage for the code. It's scheduled for commit together today.
Unfortunately nobody looked at the test much...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] advanced partition matching algorithm for partition-wise join
On Tue, May 14, 2019 at 10:00 AM Amit Langote wrote: > On 2019/05/14 13:23, Amit Langote wrote: > > Tom > > strongly objected to that idea saying that such join paths are kind of > > silly [1], even outside the context of partitionwise join. He suggested > > that we abandon partitionwise join in such cases, because having to build > > a dummy base relation for pruned partitions only to generate > silly-looking > > paths would be an ugly kludge. > > I forgot to mention that he even committed a patch to disable > partitionwise joins in such cases, which was also applied to v11 branch. > > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d70c147fa217c4bae32ac1afb86ab42d98b36fdf > > Note that there were also other reasons for committing, beside what I > described in my previous email. > > I haven't seen the actual commit, but we could use these patches to enable partition-wise join when partitions are pruned. For that the partition descriptor of the pruned partition table should be arranged as if those partitions are missing in the table itself. However, we will still need code to handle the cases when the partitions are missing on the nullable side. Tom mentioned the idea of using just projection to produce join tuples with rows on the outer side appended with null columns from the nullable side. If we can implement that, we can remove the restrictions in this patch. -- Best Wishes, Ashutosh Bapat
Re: PANIC :Call AbortTransaction when transaction id is no normal
Andres Freund writes: > On 2019-05-14 12:37:39 +0900, Michael Paquier wrote: >> Still, I would like to understand why the bootstrap process has been >> signaled to begin with, particularly for an initdb, which is not >> really something that should happen on a server where an instance >> runs. If you have a too aggressive monitoring job, you may want to >> revisit that as well, because it is able to complain just with an >> initdb. > Shutdown, timeout, resource exhaustion all seem like possible > causes. Don't think any of them warrant a core file - as the OP > explains, that'll often trigger pages etc. Yeah. The case I was thinking about was mostly "start initdb, decide I didn't want to do that, hit control-C". That cleans up without much fuss *except* if you manage to hit the window where it's running bootstrap, and then it spews this scary-looking error. It's less scary-looking with the SIG_DFL patch, which I've now pushed. regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 03, 2019 at 11:45:33AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-03 10:08:59 -0700, Mark Wong wrote: > > Ok, I think I have gdb installed now... > > Thanks! Any chance you could turn on force_parallel_mode for the other > branches it applies to too? Makes it easier to figure out whether > breakage is related to that, or independent. Slowly catching up on my collection of ppc64le animals... I still need to upgrade the build farm client (v8) on: * dhole * vulpes * wobbegong * cuon * batfish * devario * cardinalfish The following I've enabled force_parallel_mode for HEAD, 11, 10, and 9.6: * buri * urocryon * ayu * shoveler * chimaera * bonito * takin * bufflehead * elasmobranch * demoiselle * cavefish Regards, Mark
Re: Why is infinite_recurse test suddenly failing?
Mark Wong writes: > The following I've enabled force_parallel_mode for HEAD, 11, 10, and > 9.6: Thanks Mark! In theory, the stack trace we now have from shoveler proves that parallel mode has nothing to do with this, because the crash happens during planning (specifically, inlining SQL functions). I wonder though if it's possible that previous force_parallel_mode queries have had some undesirable effect on the process's stack allocation. Just grasping at straws, because it's sure not clear what's going wrong. regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
Mark Wong writes: > Slowly catching up on my collection of ppc64le animals... Oh, btw ... you didn't answer my question from before: are these animals all running on a common platform (and if so, what is that), or are they really different hardware? If they're all VMs on one machine then it might be that there's some common-mode effect from the underlying system. (Another thing I notice, now, is that these are all Linux variants; I'd been thinking you had some BSDen in there too, but now I see that none of those are ppc64. Hm.) regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 10, 2019 at 11:27:07AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-10 11:38:57 -0400, Tom Lane wrote: > > Core was generated by `postgres: debian regression [local] SELECT > > '. > > Program terminated with signal SIGSEGV, Segmentation fault. > > #0 sysmalloc (nb=8208, av=0x3fff916e0d28 ) at malloc.c:2748 > > 2748malloc.c: No such file or directory. > > #0 sysmalloc (nb=8208, av=0x3fff916e0d28 ) at malloc.c:2748 > > #1 0x3fff915bedc8 in _int_malloc (av=0x3fff916e0d28 , > > bytes=8192) at malloc.c:3865 > > #2 0x3fff915c1064 in __GI___libc_malloc (bytes=8192) at malloc.c:2928 > > #3 0x106acfd8 in AllocSetContextCreateInternal > > (parent=0x1000babdad0, name=0x1085508c "inline_function", > > minContextSize=, initBlockSize=, > > maxBlockSize=8388608) at aset.c:477 > > #4 0x103d5e00 in inline_function (funcid=20170, > > result_type=, result_collid=, > > input_collid=, funcvariadic=, > > func_tuple=, context=0x3fffe3da15d0, args=) > > at clauses.c:4459 > > #5 simplify_function (funcid=, result_type=, > > result_typmod=, result_collid=, > > input_collid=, args_p=, > > funcvariadic=, process_args=, > > allow_non_const=, context=) at clauses.c:4040 > > #6 0x103d2e74 in eval_const_expressions_mutator > > (node=0x1000babe968, context=0x3fffe3da15d0) at clauses.c:2474 > > #7 0x103511bc in expression_tree_mutator (node=, > > mutator=0x103d2b10 , > > context=0x3fffe3da15d0) at nodeFuncs.c:2893 > > > > So that lets out any theory that somehow we're getting into a weird > > control path that misses calling check_stack_depth; > > expression_tree_mutator does so for one, and it was called just nine > > stack frames down from the crash. > > Right. There's plenty places checking it... > > > > I am wondering if, somehow, the stack depth limit seen by the postmaster > > sometimes doesn't apply to its children. That would be pretty wacko > > kernel behavior, especially if it's only intermittently true. > > But we're running out of other explanations. > > I wonder if this is a SIGSEGV that actually signals an OOM > situation. Linux, if it can't actually extend the stack on-demand due to > OOM, sends a SIGSEGV. The signal has that information, but > unfortunately the buildfarm code doesn't print it. p $_siginfo would > show us some of that... > > Mark, how tight is the memory on that machine? There's about 2GB allocated: debian@postgresql-debian:~$ cat /proc/meminfo MemTotal:2080704 kB MemFree: 1344768 kB MemAvailable:1824192 kB At the moment it looks like plenty. :) Maybe I should set something up to monitor these things. > Does dmesg have any other > information (often segfaults are logged by the kernel with the code > IIRC). It's been up for about 49 days: debian@postgresql-debian:~$ uptime 14:54:30 up 49 days, 14:59, 3 users, load average: 0.00, 0.34, 1.04 I see one line from dmesg that is related to postgres: [3939350.616849] postgres[17057]: bad frame in setup_rt_frame: 3fffe3d9fe00 nip 3fff915bdba0 lr 3fff915bde9c But only that one time in 49 days up. Otherwise I see a half dozen hung_task_timeout_secs messages around jdb2 and dhclient. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Re: Why is infinite_recurse test suddenly failing?
On Tue, May 14, 2019 at 10:52:07AM -0400, Tom Lane wrote: > Mark Wong writes: > > Slowly catching up on my collection of ppc64le animals... > > Oh, btw ... you didn't answer my question from before: are these animals > all running on a common platform (and if so, what is that), or are they > really different hardware? If they're all VMs on one machine then it > might be that there's some common-mode effect from the underlying system. Sorry, I was almost there. :) These systems are provisioned with OpenStack. Additionally, a couple more (cardinalfish, devario) are using docker under that. > (Another thing I notice, now, is that these are all Linux variants; > I'd been thinking you had some BSDen in there too, but now I see > that none of those are ppc64. Hm.) Right, the BSDen I have are on different hardware. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 10, 2019 at 05:26:43PM -0400, Andrew Dunstan wrote: > > On 5/10/19 3:35 PM, Tom Lane wrote: > > Andres Freund writes: > >> On 2019-05-10 11:38:57 -0400, Tom Lane wrote: > >>> I am wondering if, somehow, the stack depth limit seen by the postmaster > >>> sometimes doesn't apply to its children. That would be pretty wacko > >>> kernel behavior, especially if it's only intermittently true. > >>> But we're running out of other explanations. > >> I wonder if this is a SIGSEGV that actually signals an OOM > >> situation. Linux, if it can't actually extend the stack on-demand due to > >> OOM, sends a SIGSEGV. The signal has that information, but > >> unfortunately the buildfarm code doesn't print it. p $_siginfo would > >> show us some of that... > >> Mark, how tight is the memory on that machine? Does dmesg have any other > >> information (often segfaults are logged by the kernel with the code > >> IIRC). > > It does sort of smell like a resource exhaustion problem, especially > > if all these buildfarm animals are VMs running on the same underlying > > platform. But why would that manifest as "you can't have a measly two > > megabytes of stack" and not as any other sort of OOM symptom? > > > > Mark, if you don't mind modding your local copies of the buildfarm > > script, I think what Andres is asking for is a pretty trivial addition > > in PGBuild/Utils.pm's sub get_stack_trace: > > > > my $cmdfile = "./gdbcmd"; > > my $handle; > > open($handle, '>', $cmdfile) || die "opening $cmdfile: $!"; > > print $handle "bt\n"; > > + print $handle "p $_siginfo\n"; > > close($handle); > > > > > > > I think we'll need to write that as: > > > print $handle 'p $_siginfo',"\n"; Ok, I have this added to everyone now. I think I also have caught up on this thread, but let me know if I missed anything. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
SEASON OF DOCS PROJECT
Hi there I am interested about the project and have gone through the project idea. But I would like to know more about the project and the organization expectations the tech writers .Apart from the skills and language mentioned..what more skills/language you are expecting from technical writers. please let me know.
Re: Inconsistent error message wording for REINDEX CONCURRENTLY
On 2019-May-09, Michael Paquier wrote:
> On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
> > Michael Paquier writes:
> >> No problem to do that. I'll brush up all that once you commit the
> >> first piece you have come up with, and reuse the new API of catalog.c
> >> you are introducing based on the table OID.
> >
> > Pushed my stuff, have at it.
>
> Thanks. Attached is what I get to after scanning the error messages
> in indexcmds.c and index.c. Perhaps you have more comments about it?
I do :-) There are a couple of "is not supported" messages that are
annoyingly similar but different:
git grep --show-function 'reindex.*supported' -- *.c
src/backend/commands/indexcmds.c=ReindexMultipleTables(const char *objectName,
ReindexObjectType objectKind,
src/backend/commands/indexcmds.c:errmsg("concurrent reindex of
system catalogs is not supported")));
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for catalog relations, skipping all")));
src/backend/commands/indexcmds.c=ReindexRelationConcurrently(Oid relationOid,
int options)
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for shared relations")));
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for catalog relations")));
It seems strange to have some cases say "cannot do foo" and other cases
say "foo is not supported". However, I think having
ReindexMultipleTables say "cannot reindex a system catalog" would be
slightly wrong (since we're not reindexing one but many) -- so it would
have to be "cannot reindex system catalogs". And in order to avoid
having two messages that are essentially identical except in number, I
propose to change the others to use the plural too. So the one you just
committed
> + /* A system catalog cannot be reindexed
> concurrently */
> + if (IsCatalogRelationOid(relationOid))
> + ereport(ERROR,
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot reindex
> a system catalog concurrently")));
would become "cannot reindex system catalogs concurrently", identical to
the one in ReindexMultipleTables.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SQL-spec incompatibilities in similar_escape() and related stuff
[ backing up to a different sub-discussion ]
Andrew Gierth writes:
> "Tom" == Tom Lane writes:
> Tom> To support the first usage, similar_escape is non-strict, and it
> Tom> takes a NULL second argument to mean '\'. This is already a SQL
> Tom> spec violation, because as far as I can tell from the spec, if you
> Tom> don't write an ESCAPE clause then there is *no* escape character;
> Tom> there certainly is not a default of '\'. However, we document this
> Tom> behavior, so I don't know if we want to change it.
> This is the same spec violation that we also have for LIKE, which also
> is supposed to have no escape character in the absense of an explicit
> ESCAPE clause.
Right. After further thought, I propose that what we ought to do is
unify LIKE, SIMILAR TO, and 3-arg SUBSTRING on a single set of behaviors
for the ESCAPE argument:
1. They are strict, ie a NULL value for the escape string produces a
NULL result. This is per spec, and we don't document anything different,
and nobody would really expect something different. (But see below
about keeping similar_escape() as a legacy compatibility function.)
2. Omitting the ESCAPE option (not possible for SUBSTRING) results in a
default of '\'. This is not per spec, but we've long documented it this
way, and frankly I'd say that it's a far more useful default than the
spec's behavior of "there is no escape character". I propose that we
just document that this is not-per-spec and move on.
3. Interpret an empty ESCAPE string as meaning "there is no escape
character". This is not per spec either (the spec would have us
throw an error) but it's our historical behavior, and it seems like
a saner approach than the way the spec wants to do it --- in particular,
there's no way to get that behavior in 3-arg SUBSTRING if we don't allow
this.
So only point 1 represents an actual behavioral change from what we've
been doing; the other two just require doc clarifications.
Now, I don't have any problem with changing what happens when somebody
actually writes "a LIKE b ESCAPE NULL"; it seems fairly unlikely that
anyone would expect that to yield a non-null result. However, we do
have a problem with the fact that the implementation is partially
exposed:
regression=# create view v1 as select f1 similar to 'x*' from text_tbl;
CREATE VIEW
regression=# \d+ v1
...
View definition:
SELECT text_tbl.f1 ~ similar_escape('x*'::text, NULL::text)
FROM text_tbl;
If we just change similar_escape() to be strict, then this view will
stop working, which is a bit hard on users who did not write anything
non-spec-compliant.
I propose therefore that we leave similar_escape in place with its
current behavior, as a compatibility measure for cases like this.
Intead, invent two new strict functions, say
similar_to_escape(pattern)
similar_to_escape(pattern, escape)
and change the parser and the implementation of SUBSTRING() to
rely on these going forward.
The net effect will be to make explicit "ESCAPE NULL" spec-compliant,
and to get rid of the performance problem from inlining failure for
substring(). All else is just doc clarifications.
Comments?
regards, tom lane
VACUUM fails to parse 0 and 1 as boolean value
Hi, VACUUM fails to parse 0 and 1 as boolean value The document for VACUUM explains boolean Specifies whether the selected option should be turned on or off. You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF, or 0 to disable it. But VACUUM fails to parse 0 and 1 as boolean value as follows. =# VACUUM (INDEX_CLEANUP 1); ERROR: syntax error at or near "1" at character 23 STATEMENT: VACUUM (INDEX_CLEANUP 1); This looks a bug. The cause of this is a lack of NumericOnly clause for vac_analyze_option_arg in gram.y. The attached patch adds such NumericOnly. The bug exists only in 12dev. Barring any objection, I will commit the patch. Regards, -- Fujii Masao vacuum_numeric_as_boolean.patch Description: Binary data
Re: VACUUM fails to parse 0 and 1 as boolean value
Hi, On 2019-05-15 02:45:21 +0900, Fujii Masao wrote: > VACUUM fails to parse 0 and 1 as boolean value > > The document for VACUUM explains > > boolean > Specifies whether the selected option should be turned on or off. > You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF, > or 0 to disable it. > > But VACUUM fails to parse 0 and 1 as boolean value as follows. > > =# VACUUM (INDEX_CLEANUP 1); > ERROR: syntax error at or near "1" at character 23 > STATEMENT: VACUUM (INDEX_CLEANUP 1); > > This looks a bug. The cause of this is a lack of NumericOnly clause > for vac_analyze_option_arg in gram.y. The attached patch > adds such NumericOnly. The bug exists only in 12dev. > > Barring any objection, I will commit the patch. Might be worth having a common rule for such options, so we don't duplicate the knowledge between different places. CCing Robert and Sawada-san, who committed / authored that code. commit 41b54ba78e8c4d64679ba4daf82e4e2efefe1922 Author: Robert Haas Date: 2019-03-29 08:22:49 -0400 Allow existing VACUUM options to take a Boolean argument. This makes VACUUM work more like EXPLAIN already does without changing the meaning of any commands that already work. It is intended to facilitate the addition of future VACUUM options that may take non-Boolean parameters or that default to false. Masahiko Sawada, reviewed by me. Discussion: http://postgr.es/m/CA+TgmobpYrXr5sUaEe_T0boabV0DSm=utsozzwcunqfleem...@mail.gmail.com Discussion: http://postgr.es/m/CAD21AoBaFcKBAeL5_++j+Vzir2vBBcF4juW7qH8b3HsQY=q...@mail.gmail.com Greetings, Andres Freund
Re: vacuumdb and new VACUUM options
On Tue, May 14, 2019 at 10:01 AM Michael Paquier wrote: > > On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote: > > Thank you! I've incorporated your comment in my branch. I'll post the > > updated version patch after the above discussion got a consensus. > > Fujii-san, any input about the way to move forward here? Beta1 is > planned for next week, hence it would be nice to progress on this > front this week. I think that we can push "--index-cleanup=BOOLEAN" version into beta1, and then change the interface of the options if we received many complaints about "--index-cleanup=BOOLEAN" from users. So this week, I'd like to review Sawada's patch and commit it if that's ok. Regards, -- Fujii Masao
Re: vacuumdb and new VACUUM options
On Thu, May 9, 2019 at 8:20 PM Masahiko Sawada wrote: > > On Thu, May 9, 2019 at 10:01 AM Michael Paquier wrote: > > > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote: > > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao > > > escreveu: > > >> The question is; we should support vacuumdb option for (1), i.e.,, > > >> something like --index-cleanup option is added? > > >> Or for (2), i.e., something like --disable-index-cleanup option is added > > >> as your patch does? Or for both? > > > > > > --index-cleanup=BOOL > > > > I agree with Euler's suggestion to have a 1-1 mapping between the > > option of vacuumdb and the VACUUM parameter > > +1. Attached the draft version patches for both options. Thanks for the patch! + if (strncasecmp(opt_str, "true", 4) != 0 && + strncasecmp(opt_str, "false", 5) != 0) Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, like VACUUM does? + char *index_cleanup; The patch would be simpler if enum trivalue is used for index_cleanup variable as the type. Regards, -- Fujii Masao
Re: PG12, PGXS and linking pgfeutils
I wrote: > I think moving fe_utils/logging.[hc] to > common/ is definitely the way to get out of this problem. I've pushed that, so Ian's problem should be gone as of HEAD. regards, tom lane
Re: Inconsistency between table am callback and table function names
On Mon, May 13, 2019 at 12:51 PM Robert Haas wrote: > On Fri, May 10, 2019 at 3:43 PM Ashwin Agrawal > wrote: > > Meant to stick the question mark in that email, somehow missed. Yes > > not planning to spend any time on it if objections. Here is the list > > of renames I wish to perform. > > > > Lets start with low hanging ones. > > > > table_rescan -> table_scan_rescan > > table_insert -> table_tuple_insert > > table_insert_speculative -> table_tuple_insert_speculative > > table_delete -> table_tuple_delete > > table_update -> table_tuple_update > > table_lock_tuple -> table_tuple_lock > > > > Below two you already mentioned no objections to rename > > table_fetch_row_version -> table_tuple_fetch_row_version > > table_get_latest_tid -> table_tuple_get_latest_tid > > > > Now, table_beginscan and table_endscan are the ones which are > > wide-spread. > > I vote to rename all the ones where the new name would contain "tuple" > and to leave the others alone. i.e. leave table_beginscan, > table_endscan, and table_rescan as they are. I think that there's > little benefit in standardizing table_rescan but not the other two, > and we seem to agree that tinkering with the other two gets into a > painful amount of churn. > Thank you. Please find the patch to rename the agreed functions. It would be good to make all consistent instead of applying exception to three functions but seems no consensus on it. Given table_ api are new, we could modify them leaving systable_ ones as is, but as objections left that as is. From 39fcc223a0aaeacf545e09cfe29b8d565d970234 Mon Sep 17 00:00:00 2001 From: Ashwin Agrawal Date: Tue, 14 May 2019 10:10:21 -0700 Subject: [PATCH] Rename table am wrappers to match table am callback names. Now all the table wrapper functions match the "table_" theme, except table_beginscan(), table_endscan() and table_rescan(). The exception was applied and decision is not to rename the three functions based on discussion to avoid code churn. --- src/backend/access/heap/heapam.c | 10 ++-- src/backend/access/table/tableam.c | 46 +++ src/backend/commands/copy.c| 12 ++-- src/backend/commands/createas.c| 14 ++--- src/backend/commands/matview.c | 14 ++--- src/backend/commands/tablecmds.c | 4 +- src/backend/commands/trigger.c | 12 ++-- src/backend/executor/execMain.c| 8 +-- src/backend/executor/execReplication.c | 16 +++--- src/backend/executor/nodeLockRows.c| 8 +-- src/backend/executor/nodeModifyTable.c | 78 - src/backend/executor/nodeTidscan.c | 4 +- src/backend/utils/adt/tid.c| 4 +- src/include/access/tableam.h | 80 +- 14 files changed, 155 insertions(+), 155 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index ec9853603fd..86a964c4f48 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1856,12 +1856,12 @@ ReleaseBulkInsertStatePin(BulkInsertState bistate) * The new tuple is stamped with current transaction ID and the specified * command ID. * - * See table_insert for comments about most of the input flags, except that + * See table_tuple_insert for comments about most of the input flags, except that * this routine directly takes a tuple rather than a slot. * * There's corresponding HEAP_INSERT_ options to all the TABLE_INSERT_ * options, and there additionally is HEAP_INSERT_SPECULATIVE which is used to - * implement table_insert_speculative(). + * implement table_tuple_insert_speculative(). * * On return the header fields of *tup are updated to match the stored tuple; * in particular tup->t_self receives the actual TID where the tuple was @@ -2434,7 +2434,7 @@ xmax_infomask_changed(uint16 new_infomask, uint16 old_infomask) /* * heap_delete - delete a tuple * - * See table_delete() for an explanation of the parameters, except that this + * See table_tuple_delete() for an explanation of the parameters, except that this * routine directly takes a tuple rather than a slot. * * In the failure cases, the routine fills *tmfd with the tuple's t_ctid, @@ -2880,7 +2880,7 @@ simple_heap_delete(Relation relation, ItemPointer tid) /* * heap_update - replace a tuple * - * See table_update() for an explanation of the parameters, except that this + * See table_tuple_update() for an explanation of the parameters, except that this * routine directly takes a tuple rather than a slot. * * In the failure cases, the routine fills *tmfd with the tuple's t_ctid, @@ -3951,7 +3951,7 @@ get_mxact_status_for_lock(LockTupleMode mode, bool is_update) * *buffer: set to buffer holding tuple (pinned but not locked at exit) * *tmfd: filled in failure cases (see below) * - * Function results are the same as the ones for table_lock_tuple(). + * Function results are the same as the ones for table_tuple_lock(). * * In the failure cases
Re: Inconsistency between table am callback and table function names
On Wed, May 8, 2019 at 5:05 PM Ashwin Agrawal wrote: > Not having consistency is the main aspect I wish to bring to > attention. Like for some callback functions the comment is > > /* see table_insert() for reference about parameters */ > void(*tuple_insert) (Relation rel, TupleTableSlot *slot, > CommandId cid, int options, > struct BulkInsertStateData *bistate); > > /* see table_insert_speculative() for reference about parameters > */ > void(*tuple_insert_speculative) (Relation rel, > TupleTableSlot *slot, > CommandId cid, > int options, > struct > BulkInsertStateData *bistate, > uint32 specToken); > > Whereas for some other callbacks the parameter explanation exist in > both the places. Seems we should be consistent. > I feel in long run becomes pain to keep them in sync as comments > evolve. Like for example > > /* > * Estimate the size of shared memory needed for a parallel scan > of this > * relation. The snapshot does not need to be accounted for. > */ > Size(*parallelscan_estimate) (Relation rel); > > parallescan_estimate is not having snapshot argument passed to it, but > table_parallescan_estimate does. So, this way chances are high they > going out of sync and missing to modify in both the places. Agree > though on audience being different for both. So, seems going with the > refer XXX for parameters seems fine approach for all the callbacks and > then only specific things to flag out for the AM layer to be aware can > live above the callback function. > The topic of consistency for comment style for all wrappers seems got lost with other discussion, would like to seek opinion on the same.
Re: Adding a test for speculative insert abort case
Hi, On 2019-05-10 14:40:38 -0700, Andres Freund wrote: > On 2019-05-01 11:41:48 -0700, Andres Freund wrote: > > I'm imagining something like > > > > if (pg_try_advisory_xact_lock(1)) > > pg_advisory_xact_lock(2); > > else > > pg_advisory_xact_lock(1); > > > > in t1_lock_func. If you then make the session something roughly like > > > > s1: pg_advisory_xact_lock(1); > > s1: pg_advisory_xact_lock(2); > > > > s2: upsert t1 > > s1: pg_advisory_xact_unlock(1); > > s2: > > s2: > > s1: insert into t1 values(1, 'someval'); > > s1: pg_advisory_xact_unlock(2); > > s2: > > s2: spec-conflict > > Needed to be slightly more complicated than that, but not that much. See > the attached test. What do you think? > > I think we should apply something like this (minus the WARNING, of > course). It's a bit complicated, but it seems worth covering this > special case. And pushed. Let's see what the buildfarm says. Regards, Andres
Re: Match table_complete_speculative() code to comment
Hi,
On 2019-04-30 11:53:38 -0700, Ashwin Agrawal wrote:
> Comment for table_complete_speculative() says
>
> /*
> * Complete "speculative insertion" started in the same transaction.
> If
> * succeeded is true, the tuple is fully inserted, if false, it's
> removed.
> */
> static inline void
> table_complete_speculative(Relation rel, TupleTableSlot *slot,
>uint32 specToken, bool succeeded)
> {
> rel->rd_tableam->tuple_complete_speculative(rel, slot, specToken,
> succeeded);
> }
>
> but code really refers to succeeded as failure. Since that argument is
> passed as specConflict, means conflict happened and hence the tuple
> should be removed. It would be better to fix the code to match the
> comment as in AM layer its better to deal with succeeded to finish the
> insertion and not other way round.
>
> diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 4d179881f27..241639cfc20 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -282,7 +282,7 @@ heapam_tuple_complete_speculative(Relation
> relation, TupleTableSlot *slot,
> HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true,
> &shouldFree);
>
> /* adjust the tuple's state accordingly */
> - if (!succeeded)
> + if (succeeded)
> heap_finish_speculative(relation, &slot->tts_tid);
> else
> heap_abort_speculative(relation, &slot->tts_tid);
> diff --git a/src/backend/executor/nodeModifyTable.c
> b/src/backend/executor/nodeModifyTable.c
> index 444c0c05746..d545bbce8a2 100644
> --- a/src/backend/executor/nodeModifyTable.c
> +++ b/src/backend/executor/nodeModifyTable.c
> @@ -556,7 +556,7 @@ ExecInsert(ModifyTableState *mtstate,
>
> /* adjust the tuple's state accordingly */
> table_complete_speculative(resultRelationDesc, slot,
> -
> specToken, specConflict);
> +
> specToken, !specConflict);
And pushed, as
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=aa4b8c61d2cd57b53be03defb04d59b232a0e150
with the part that wasn't covered by tests now covered by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=08e2edc0767ab6e619970f165cb34d4673105f23
Greetings,
Andres Freund
Re: Table AM callback table_complete_speculative()'s succeeded argument is reversed
Hi,
On 2019-05-14 07:06:34 -0700, Andres Freund wrote:
> On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas wrote:
> >The 'succeeded' argument seems backwards here:
> >
> >> static void
> >> heapam_tuple_complete_speculative(Relation relation, TupleTableSlot
> >*slot,
> >> uint32
> >> spekToken, bool succeeded)
> >> {
> >>boolshouldFree = true;
> >>HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
> >>
> >>/* adjust the tuple's state accordingly */
> >>if (!succeeded)
> >>heap_finish_speculative(relation, &slot->tts_tid);
> >>else
> >>heap_abort_speculative(relation, &slot->tts_tid);
> >>
> >>if (shouldFree)
> >>pfree(tuple);
> >> }
> >
> >According to the comments, if "succeeded = true", the insertion is
> >completed, and otherwise it's killed. It works, because the only caller
> >
> >is also passing the argument wrong.
>
> Thanks for finding.
>
>
> >Barring objections, I'll push the attached patch to fix that.
>
> Please hold off - your colleagues found this before, and I worked on getting
> test coverage for the code. It's scheduled for commit together today.
> Unfortunately nobody looked at the test much...
\
And pushed, as
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=aa4b8c61d2cd57b53be03defb04d59b232a0e150
with the part that wasn't covered by tests now covered by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=08e2edc0767ab6e619970f165cb34d4673105f23
Greetings,
Andres Freund
Re: ldapbindpasswdfile
> On 14 May 2019, at 03:49, Thomas Munro wrote: > I propose a new option $SUBJECT so that users can at least add a level of > indirection and put the password in a file. +1, seems like a reasonable option to give. > Draft patch attached. I might be a bit thick, but this is somewhat hard to parse IMO: +File containing the password for user to bind to the directory with to +perform the search when doing search+bind authentication To add a little bit more security around this, does it make sense to check (on unix filesystems) that the file isn’t world readable/editable? + fd = OpenTransientFile(path, O_RDONLY); + if (fd < 0) + return -1; cheers ./daniel
Re: Inconsistency between table am callback and table function names
On 2019-May-14, Ashwin Agrawal wrote: > Thank you. Please find the patch to rename the agreed functions. It would > be good to make all consistent instead of applying exception to three > functions but seems no consensus on it. Given table_ api are new, we could > modify them leaving systable_ ones as is, but as objections left that as is. Hmm .. I'm surprised to find out that we only have one caller of simple_table_insert, simple_table_delete, simple_table_update. I'm not sure I agree to the new names those got in the renaming patch, since they're not really part of table AM proper ... do we really want to offer those as separate entry points? Why not just remove those routines? Somewhat related: it's strange that CatalogTupleUpdate etc use simple_heap_update instead of the tableam variants wrappers (I suppose that's either because of bootstrapping concerns, or performance). Would it be too strange to have CatalogTupleInsert call heap_insert() directly, and do away with simple_heap_insert? (Equivalently for update, delete). I think those wrappers made perfect sense when we had simple_heap_insert all around the place ... but now that we introduced the CatalogTupleFoo wrappers, I don't think it does any longer. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Inconsistency between table am callback and table function names
Hi, On 2019-05-14 16:27:47 -0400, Alvaro Herrera wrote: > On 2019-May-14, Ashwin Agrawal wrote: > > > Thank you. Please find the patch to rename the agreed functions. It would > > be good to make all consistent instead of applying exception to three > > functions but seems no consensus on it. Given table_ api are new, we could > > modify them leaving systable_ ones as is, but as objections left that as is. > > Hmm .. I'm surprised to find out that we only have one caller of > simple_table_insert, simple_table_delete, simple_table_update. I'm not > sure I agree to the new names those got in the renaming patch, since > they're not really part of table AM proper ... do we really want to > offer those as separate entry points? Why not just remove those routines? I don't think it'd be better if execReplication.c has them inline - we'd just have the exact same code inline. There's plenty extension out there that use simple_heap_*, and without such wrappers, they'll all have to copy the contents of simple_table_* too. Also we'll probably want to switch CatalogTuple* over to them at some point. > Somewhat related: it's strange that CatalogTupleUpdate etc use > simple_heap_update instead of the tableam variants wrappers (I suppose > that's either because of bootstrapping concerns, or performance). It's because the callers currently expect to work with heap tuples, rather than slots. And changing that would have been a *LOT* of work (as in: prohibitively much for v12). I didn't want to create a slot for each insertion, because that'd make them slower. But as Robert said on IM (discussing something else), we already create a slot in most cases, via CatalogIndexInsert(). Not sure if HOT updates and deletes are common enough to make the slot creation in those cases measurable. > Would it be too strange to have CatalogTupleInsert call heap_insert() > directly, and do away with simple_heap_insert? (Equivalently for > update, delete). I think those wrappers made perfect sense when we had > simple_heap_insert all around the place ... but now that we introduced > the CatalogTupleFoo wrappers, I don't think it does any longer. I don't really see the advantage. Won't that just break a lot of code that will continue to work otherwise, as long as you just use heap tables? With the sole benefit of moving a bit of code from one place to another? Greetings, Andres Freund
PSA: New intel MDS vulnerability mitigations cause measurable slowdown
Hi, There's a new set of CPU vulnerabilities, so far only affecting intel CPUs. Cribbing from the linux-kernel announcement I'm referring to https://xenbits.xen.org/xsa/advisory-297.html for details. The "fix" is for the OS to perform some extra mitigations: https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html https://www.kernel.org/doc/html/latest/x86/mds.html#mds *And* SMT/hyperthreading needs to be disabled, to be fully safe. Fun. I've run a quick pgbench benchmark: *Without* disabling SMT, for readonly pgbench, I'm seeing regressions between 7-11%, depending on the size of shared_buffers (and some runtime variations). That's just on my laptop, with an i7-6820HQ / Haswell CPU. I'd be surprised if there weren't adversarial loads with bigger slowdowns - what gets more expensive with the mitigations is syscalls. Most OSs / distributions either have rolled these changes out already, or will do so soon. So it's likely that most of us and our users will be affected by this soon. At least on linux the part of the mitigation that makes syscalls slower (blowing away buffers at the end of a sycall) is enabled by default, but SMT is not disabled by default. Greetings, Andres Freund
Re: vacuumdb and new VACUUM options
On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote: > + if (strncasecmp(opt_str, "true", 4) != 0 && > + strncasecmp(opt_str, "false", 5) != 0) > > Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, > like VACUUM does? I am wondering, in order to keep this patch simple, if you shouldn't accept any value and just let the parsing logic on the backend side do all the work. That's what we do for other things like the connection parameter replication for example, and there is no need to mimic a boolean parsing equivalent on the frontend with something like check_bool_str() as presented in the patch. The main downside is that the error message gets linked to VACUUM and not vacuumdb. Another thing which you may be worth looking at would be to make parse_bool() frontend aware, where pg_strncasecmp() is actually available. -- Michael signature.asc Description: PGP signature
Re: VACUUM fails to parse 0 and 1 as boolean value
On Tue, May 14, 2019 at 10:52:23AM -0700, Andres Freund wrote: > Might be worth having a common rule for such options, so we don't > duplicate the knowledge between different places. > > CCing Robert and Sawada-san, who committed / authored that code. Hmn. I think that Robert's commit is right to rely on defGetBoolean() for option parsing. That's what we use for anything from CREATE EXTENSION to CREATE SUBSCRIPTION, etc. -- Michael signature.asc Description: PGP signature
Re: VACUUM fails to parse 0 and 1 as boolean value
Hi, On 2019-05-15 08:20:33 +0900, Michael Paquier wrote: > On Tue, May 14, 2019 at 10:52:23AM -0700, Andres Freund wrote: > > Might be worth having a common rule for such options, so we don't > > duplicate the knowledge between different places. > > > > CCing Robert and Sawada-san, who committed / authored that code. > > Hmn. I think that Robert's commit is right to rely on defGetBoolean() > for option parsing. That's what we use for anything from CREATE > EXTENSION to CREATE SUBSCRIPTION, etc. That seems like a separate angle? What does that have to do with accepting 0/1 in the grammar? I mean, EXPLAIN also uses defGetBoolean(), while accepting NumericOnly for the option values? Greetings, Andres Freund
Re: VACUUM fails to parse 0 and 1 as boolean value
On Wed, May 15, 2019 at 08:20:33AM +0900, Michael Paquier wrote: > Hmn. I think that Robert's commit is right to rely on defGetBoolean() > for option parsing. That's what we use for anything from CREATE > EXTENSION to CREATE SUBSCRIPTION, etc. And I need more coffee at this time of the day... Because I have not looked at the proposed patch. The patch of Fujii-san does its job as far as it goes, but we have more parsing nodes with the same logic: - explain_option_arg, which is the same. - copy_generic_opt_arg, which shares the same root. So there is room for a common rule, still it does not impact that many places. I would have believed that more commands use that. -- Michael signature.asc Description: PGP signature
Re: PG12, PGXS and linking pgfeutils
On 5/15/19 3:38 AM, Tom Lane wrote: I wrote: I think moving fe_utils/logging.[hc] to common/ is definitely the way to get out of this problem. I've pushed that, so Ian's problem should be gone as of HEAD. Thanks, that resolves the issue! Regards Ian Barwick -- Ian Barwick https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: pgsql: Compute XID horizon for page level index vacuum on primary.
On Thu, May 2, 2019 at 5:10 AM Robert Haas wrote: > On Wed, May 1, 2019 at 12:50 PM Tom Lane wrote: > > > Not strongly enough to argue about it very hard. The current behavior > > > is a little weird, but it's a long way from being the weirdest thing > > > we ship, and it appears that we have no tangible evidence that it > > > causes a problem in practice. > > > > I think there's nothing that fails to suck about a hardwired "+ 10". > > It avoids a performance regression without adding another GUC. > > That may not be enough reason to keep it like that, but it is one > thing that does fail to suck. This is listed as an open item to resolve for 12. IIUC the options on the table are: 1. Do nothing, and ship with effective_io_concurrency + 10. 2. Just use effective_io_concurrency without the hardwired boost. 3. Switch to a new GUC maintenance_io_concurrency (or some better name). The rationale for using a different number is that this backend is working on behalf of multiple sessions, so you might want to give it some more juice, much like maintenance_work_mem. I vote for option 3. I have no clue how to set it, but at least users have a fighting chance of experimenting and figuring it out that way. I volunteer to write the patch if we get a consensus. -- Thomas Munro https://enterprisedb.com
Re: pgsql: Compute XID horizon for page level index vacuum on primary.
Hi, On 2019-05-15 12:01:07 +1200, Thomas Munro wrote: > On Thu, May 2, 2019 at 5:10 AM Robert Haas wrote: > > On Wed, May 1, 2019 at 12:50 PM Tom Lane wrote: > > > > Not strongly enough to argue about it very hard. The current behavior > > > > is a little weird, but it's a long way from being the weirdest thing > > > > we ship, and it appears that we have no tangible evidence that it > > > > causes a problem in practice. > > > > > > I think there's nothing that fails to suck about a hardwired "+ 10". > > > > It avoids a performance regression without adding another GUC. > > > > That may not be enough reason to keep it like that, but it is one > > thing that does fail to suck. > > This is listed as an open item to resolve for 12. IIUC the options on > the table are: > > 1. Do nothing, and ship with effective_io_concurrency + 10. > 2. Just use effective_io_concurrency without the hardwired boost. > 3. Switch to a new GUC maintenance_io_concurrency (or some better name). > > The rationale for using a different number is that this backend is > working on behalf of multiple sessions, so you might want to give it > some more juice, much like maintenance_work_mem. > > I vote for option 3. I have no clue how to set it, but at least users > have a fighting chance of experimenting and figuring it out that way. > I volunteer to write the patch if we get a consensus. I'd personally, unsurprisingly perhaps, go with 1 for v12. I think 3 is also a good option - it's easy to imagine to later use it for for VACUUM, ANALYZE and the like. I think 2 is a bad idea. Greetings, Andres Freund
Re: PSA: New intel MDS vulnerability mitigations cause measurable slowdown
On Wed, May 15, 2019 at 10:31 AM Andres Freund wrote: > *Without* disabling SMT, for readonly pgbench, I'm seeing regressions > between 7-11%, depending on the size of shared_buffers (and some runtime > variations). That's just on my laptop, with an i7-6820HQ / Haswell CPU. > I'd be surprised if there weren't adversarial loads with bigger > slowdowns - what gets more expensive with the mitigations is syscalls. Yikes. This all in warm shared buffers, right? So effectively this is the cost of recvfrom() and sendto() going up? Did you use -M prepared? If not, there would also be a couple of lseek(SEEK_END) calls in between for planning... I wonder how many more syscall-taxing mitigations we need before relation size caching pays off. -- Thomas Munro https://enterprisedb.com
Re: PSA: New intel MDS vulnerability mitigations cause measurable slowdown
Hi, On 2019-05-15 12:52:47 +1200, Thomas Munro wrote: > On Wed, May 15, 2019 at 10:31 AM Andres Freund wrote: > > *Without* disabling SMT, for readonly pgbench, I'm seeing regressions > > between 7-11%, depending on the size of shared_buffers (and some runtime > > variations). That's just on my laptop, with an i7-6820HQ / Haswell CPU. > > I'd be surprised if there weren't adversarial loads with bigger > > slowdowns - what gets more expensive with the mitigations is syscalls. > > Yikes. This all in warm shared buffers, right? Not initially, but it ought to warm up quite quickly. I ran something boiling down to pgbench -q -i -s 200; psql -c 'vacuum (freeze, analyze, verbose)'; pgbench -n -S -c 32 -j 32 -S -M prepared -T 100 -P1. As both pgbench -i's COPY and VACUUM use ringbuffers, initially s_b will effectively be empty. > So effectively this is the cost of recvfrom() and sendto() going up? Plus epoll_wait(). And read(), for the cases where s_b was smaller than the data. > Did you use -M prepared? Yes. > If not, there would also be a couple of lseek(SEEK_END) calls in > between for planning... I wonder how many more syscall-taxing > mitigations we need before relation size caching pays off. Yea, I suspect we're going to have to go there soon for a number of reasons. - Andres
Re: PSA: New intel MDS vulnerability mitigations cause measurable slowdown
Hi,
On 2019-05-14 15:30:52 -0700, Andres Freund wrote:
> There's a new set of CPU vulnerabilities, so far only affecting intel
> CPUs. Cribbing from the linux-kernel announcement I'm referring to
> https://xenbits.xen.org/xsa/advisory-297.html
> for details.
>
> The "fix" is for the OS to perform some extra mitigations:
> https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html
> https://www.kernel.org/doc/html/latest/x86/mds.html#mds
>
> *And* SMT/hyperthreading needs to be disabled, to be fully safe.
>
> Fun.
>
> I've run a quick pgbench benchmark:
>
> *Without* disabling SMT, for readonly pgbench, I'm seeing regressions
> between 7-11%, depending on the size of shared_buffers (and some runtime
> variations). That's just on my laptop, with an i7-6820HQ / Haswell CPU.
> I'd be surprised if there weren't adversarial loads with bigger
> slowdowns - what gets more expensive with the mitigations is syscalls.
The profile after the mitigations looks like:
+3.62% postgres [kernel.vmlinux] [k] do_syscall_64
+2.99% postgres postgres [.] _bt_compare
+2.76% postgres postgres [.]
hash_search_with_hash_value
+2.33% postgres [kernel.vmlinux] [k] entry_SYSCALL_64
+1.69% pgbench [kernel.vmlinux] [k] do_syscall_64
+1.61% postgres postgres [.] AllocSetAlloc
1.41% postgres postgres [.] PostgresMain
+1.22% pgbench [kernel.vmlinux] [k] entry_SYSCALL_64
+1.11% postgres postgres [.] LWLockAcquire
+0.86% postgres postgres [.] PinBuffer
+0.80% postgres postgres [.] LockAcquireExtended
+0.78% postgres [kernel.vmlinux] [k] psi_task_change
0.76% pgbench pgbench [.] threadRun
0.69% postgres postgres [.] LWLockRelease
+0.69% postgres postgres [.] SearchCatCache1
0.66% postgres postgres [.] LockReleaseAll
+0.65% postgres postgres [.] GetSnapshotData
+0.58% postgres postgres [.] hash_seq_search
0.54% postgres postgres [.] hash_search
+0.53% postgres [kernel.vmlinux] [k] __switch_to
+0.53% postgres postgres [.] hash_any
0.52% pgbench libpq.so.5.12[.] pqParseInput3
0.50% pgbench [kernel.vmlinux] [k] do_raw_spin_lock
where do_syscall_64 show this instruction profile:
│ static __always_inline bool arch_static_branch_jump(struct
static_key *key, bool branch)
│ {
│ asm_volatile_goto("1:"
1.58 │ ↓ jmpq bd
│ mds_clear_cpu_buffers():
│ * Works with any segment selector, but a valid writable
│ * data segment is the fastest variant.
│ *
│ * "cc" clobber is required because VERW modifies ZF.
│ */
│ asm volatile("verw %[ds]" : : [ds] "m" (ds) : "cc");
77.38 │ verw 0x13fea53(%rip)# 82400ee0
│ do_syscall_64():
│ }
│
│ syscall_return_slowpath(regs);
│ }
13.18 │ bd: pop%rbx
0.08 │ pop%rbp
│ ← retq
│ nr = syscall_trace_enter(regs);
│ c0: mov%rbp,%rdi
│ → callq syscall_trace_enter
Where verw is the instruction that was recycled to now have the
side-effect of flushing CPU buffers.
Greetings,
Andres Freund
Re: VACUUM fails to parse 0 and 1 as boolean value
On Wed, May 15, 2019 at 2:52 AM Andres Freund wrote: > > Hi, > > On 2019-05-15 02:45:21 +0900, Fujii Masao wrote: > > VACUUM fails to parse 0 and 1 as boolean value > > > > The document for VACUUM explains > > > > boolean > > Specifies whether the selected option should be turned on or off. > > You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF, > > or 0 to disable it. > > > > But VACUUM fails to parse 0 and 1 as boolean value as follows. > > > > =# VACUUM (INDEX_CLEANUP 1); > > ERROR: syntax error at or near "1" at character 23 > > STATEMENT: VACUUM (INDEX_CLEANUP 1); > > > > This looks a bug. The cause of this is a lack of NumericOnly clause > > for vac_analyze_option_arg in gram.y. The attached patch > > adds such NumericOnly. The bug exists only in 12dev. Thank you for reporting and the patch. > > > > Barring any objection, I will commit the patch. > > Might be worth having a common rule for such options, so we don't > duplicate the knowledge between different places. +1 for committing this patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: Inconsistent error message wording for REINDEX CONCURRENTLY
On Tue, May 14, 2019 at 11:32:52AM -0400, Alvaro Herrera wrote:
> I do :-)
And actually I am happy to see somebody raising that point. The
current log messages are quite inconsistent for a couple of years now
but I did not bother changing anything other than the new strings per
the feedback I got until, well, yesterday.
> It seems strange to have some cases say "cannot do foo" and other cases
> say "foo is not supported". However, I think having
> ReindexMultipleTables say "cannot reindex a system catalog" would be
> slightly wrong (since we're not reindexing one but many) -- so it would
> have to be "cannot reindex system catalogs". And in order to avoid
> having two messages that are essentially identical except in number, I
> propose to change the others to use the plural too. So the one you just
> committed
>
> would become "cannot reindex system catalogs concurrently", identical to
> the one in ReindexMultipleTables.
There are also a couple of similar, much older, error messages in
index_create() for concurrent creation. Do you think that these
should be changed? I can see benefits for translators to unify things
a bit more, but these do not directly apply to REINDEX, and all
messages are a bit different depending on the context. One argument
to change them is that they don't comply with the project style as
they use full sentences.
Perhaps something like the attached for the REINDEX portion would make
the world a better place? What do you think?
--
Michael
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7e7c03ef12..8501b29b0a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2485,7 +2485,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex of system catalogs is not supported")));
+ errmsg("cannot reindex system catalogs concurrently")));
/*
* Get OID of object to reindex, being the database currently being used
@@ -2599,7 +2599,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
if (!concurrent_warning)
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("concurrent reindex is not supported for catalog relations, skipping all")));
+ errmsg("cannot reindex system catalogs concurrently, skipping all")));
concurrent_warning = true;
continue;
}
@@ -2747,7 +2747,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex a system catalog concurrently")));
+ errmsg("cannot reindex system catalogs concurrently")));
/* Open relation to get its indexes */
heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
@@ -2841,7 +2841,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex a system catalog concurrently")));
+ errmsg("cannot reindex system catalogs concurrently")));
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
@@ -2862,7 +2862,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* see reindex_relation() */
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
+ errmsg("cannot reindex partitioned tables, skipping \"%s\"",
get_rel_name(relationOid;
return false;
default:
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index c8bc6be061..fda9878f25 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2045,7 +2045,7 @@ REINDEX TABLE concur_reindex_part_10;
WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
NOTICE: table "concur_reindex_part_10" has no indexes
REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
+WARNING: cannot reindex partitioned tables, skipping "concur_reindex_part_10"
NOTICE: table "concur_reindex_part_10" has no indexes
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
@@ -2093,19 +2093,19 @@ REINDEX TABLE CONCURRENTLY concur_reindex_tab;
ERROR: REINDEX CONCURRENTLY cannot run inside a transaction block
COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
-ERROR: cannot reindex a system catalog concurrently
+ERROR: cannot reindex system catalogs conc
Re: vacuumdb and new VACUUM options
On Wed, May 15, 2019 at 7:51 AM Michael Paquier wrote: > > On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote: > > + if (strncasecmp(opt_str, "true", 4) != 0 && > > + strncasecmp(opt_str, "false", 5) != 0) > > > > Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, > > like VACUUM does? > > I am wondering, in order to keep this patch simple, if you shouldn't > accept any value and just let the parsing logic on the backend side > do all the work. That's what we do for other things like the > connection parameter replication for example, and there is no need to > mimic a boolean parsing equivalent on the frontend with something like > check_bool_str() as presented in the patch. The main downside is that > the error message gets linked to VACUUM and not vacuumdb. I might be missing something but if the frontend code doesn't check arguments and we let the backend parsing logic do all the work then it allows user to execute an arbitrary SQL command via vacuumdb. > > Another thing which you may be worth looking at would be to make > parse_bool() frontend aware, where pg_strncasecmp() is actually > available. Or how about add a function that parse a boolean string value, as a common routine among frontend programs, maybe in common.c or fe_utils? We results in having the duplicate code between frontend and backend but it may be less side effects than making parse_bool available on frontend code. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: vacuumdb and new VACUUM options
Hi, On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > I might be missing something but if the frontend code doesn't check > arguments and we let the backend parsing logic do all the work then it > allows user to execute an arbitrary SQL command via vacuumdb. But, so what? The user could just have used psql to do so? Greetings, Andres Freund
Re: vacuumdb and new VACUUM options
On Wed, May 15, 2019 at 11:45 AM Andres Freund wrote: > > Hi, > > On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > > I might be missing something but if the frontend code doesn't check > > arguments and we let the backend parsing logic do all the work then it > > allows user to execute an arbitrary SQL command via vacuumdb. > > But, so what? The user could just have used psql to do so? Indeed. It shouldn't be a problem and we even now can do that by specifying for example --table="t(c1);select 1" but doesn't work. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
wal_consistency_checking clean on HEAD (f4125278)
Hi all, I have just finished my annual set of checks with wal_consistency_checking enabled based on f4125278, and I am seeing no failures when replaying comparison pages on a standby. Thanks, -- Michael signature.asc Description: PGP signature
Re: Emacs vs pg_indent's weird indentation for function declarations
Thomas Munro writes:
> I tried teaching pgindent's post_indent subroutine to unmangle the
> multi-line declarations it mangles. That produces correct
> indentation! But can also produce lines that exceed the column limit
> we would normally wrap at (of course, because pg_bsd_indent had less
> whitespace on the left when it made wrapping decisions). Doh.
> Attached for posterity, but it's useless.
> So I think pg_bsd_indent itself needs to be fixed. I think I know
> where the problem is. lexi.c isn't looking far ahead enough to
> recognise multi-line function declarations:
I experimented with fixing this. I was able to get pg_bsd_indent to
distinguish multi-line function declarations from definitions, but it
turns out that it doesn't help your concern about the lines being too
long after re-indenting. Contrary to what you imagine above, it seems
pg_bsd_indent will not reflow argument lists, regardless of whether it
thinks there needs to be more or less leading whitespace. I'm a bit
surprised that -bc doesn't cause that to happen, but it doesn't (and I'm
not sure we'd really want to force one-parameter-per-line, anyway).
Anyway, the attached hasty-and-undercommented change to pg_bsd_indent
allows removal of the "Move prototype names to the same line as return
type" hack in pgindent, and we then get prototypes with properly
lined-up arguments, but we'll have a lot of places with over-length
lines needing manual fixing. Unless somebody wants to find where to
fix that in pg_bsd_indent, but I've had my fill of looking at that
spaghetti code for today.
regards, tom lane
diff --git a/indent.h b/indent.h
index 0fffd89..1708dbc 100644
--- a/indent.h
+++ b/indent.h
@@ -41,6 +41,8 @@ void diag2(int, const char *);
void diag3(int, const char *, int);
void diag4(int, const char *, int, int);
void dump_line(void);
+int lookahead(void);
+void lookahead_reset(void);
void fill_buffer(void);
void parse(int);
void pr_comment(void);
diff --git a/io.c b/io.c
index df11094..8d13a52 100644
--- a/io.c
+++ b/io.c
@@ -51,6 +51,13 @@ static char sccsid[] = "@(#)io.c 8.1 (Berkeley) 6/6/93";
int comment_open;
static int paren_target;
+
+static char *lookahead_buf; /* malloc'd buffer, or NULL initially */
+static char *lookahead_buf_end; /* end+1 of allocated space */
+static char *lookahead_start; /* => next char for fill_buffer() to fetch */
+static char *lookahead_ptr; /* => next char for lookahead() to fetch */
+static char *lookahead_end; /* last+1 valid char in lookahead_buf */
+
static int pad_output(int current, int target);
void
@@ -252,6 +259,58 @@ compute_label_target(void)
: ps.ind_size * (ps.ind_level - label_offset) + 1;
}
+/*
+ * Read data ahead of what has been collected into in_buffer.
+ *
+ * Successive calls get further and further ahead, until we hit EOF.
+ * Call lookahead_reset to rescan from just beyond in_buffer.
+ */
+int
+lookahead(void)
+{
+while (lookahead_ptr >= lookahead_end) {
+ int i = getc(input);
+
+ if (i == EOF)
+ return i;
+ if (i == '\0')
+ continue; /* fill_buffer drops nulls, so do we */
+
+ if (lookahead_end >= lookahead_buf_end) {
+ /* Need to allocate or enlarge lookahead_buf */
+ char *new_buf;
+ size_t req;
+
+ if (lookahead_buf == NULL) {
+ req = 64;
+ new_buf = malloc(req);
+ } else {
+ req = (lookahead_buf_end - lookahead_buf) * 2;
+ new_buf = realloc(lookahead_buf, req);
+ }
+ if (new_buf == NULL)
+ errx(1, "too much lookahead required");
+ lookahead_start = new_buf + (lookahead_start - lookahead_buf);
+ lookahead_ptr = new_buf + (lookahead_ptr - lookahead_buf);
+ lookahead_end = new_buf + (lookahead_end - lookahead_buf);
+ lookahead_buf = new_buf;
+ lookahead_buf_end = new_buf + req;
+ }
+
+ *lookahead_end++ = i;
+}
+return (unsigned char) *lookahead_ptr++;
+}
+
+/*
+ * Reset so that lookahead() will again scan from just beyond what's in
+ * in_buffer.
+ */
+void
+lookahead_reset(void)
+{
+lookahead_ptr = lookahead_start;
+}
/*
* Copyright (C) 1976 by the Board of Trustees of the University of Illinois
@@ -293,11 +352,16 @@ fill_buffer(void)
p = in_buffer + offset;
in_buffer_limit = in_buffer + size - 2;
}
- if ((i = getc(f)) == EOF) {
- *p++ = ' ';
- *p++ = '\n';
- had_eof = true;
- break;
+ if (lookahead_start < lookahead_end) {
+ i = (unsigned char) *lookahead_start++;
+ } else {
+ lookahead_start = lookahead_ptr = lookahead_end = lookahead_buf;
+ if ((i = getc(f)) == EOF) {
+ *p++ = ' ';
+ *p++ = '\n';
+ had_eof = true;
+ break;
+ }
}
if (i != '\0')
*p++ = i;
diff --git a/lexi.c b/lexi.c
index 3c7bfef..e637e1a 100644
--- a/lexi.c
+++ b/lexi.c
@@ -148,6 +148,39 @@ strcmp_type(const void *e1, const void *e2)
return (strcmp(e1, *(const char * const *)e2));
}
+/*
+ * Scan over a function argument declaration list, then see if it is
+ * followed by ';' or ',' indicating that it's just a prototype.
+ *
+
Re: vacuumdb and new VACUUM options
On Wed, May 15, 2019 at 1:01 PM Masahiko Sawada wrote: > > On Wed, May 15, 2019 at 11:45 AM Andres Freund wrote: > > > > Hi, > > > > On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > > > I might be missing something but if the frontend code doesn't check > > > arguments and we let the backend parsing logic do all the work then it > > > allows user to execute an arbitrary SQL command via vacuumdb. > > > > But, so what? The user could just have used psql to do so? > > Indeed. It shouldn't be a problem and we even now can do that by > specifying for example --table="t(c1);select 1" but doesn't work. > I've attached new version patch that takes the way to let the backend parser do all work. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center v3-0001-Add-index-cleanup-option-to-vacuumdb.patch Description: Binary data v3-0002-Add-truncate-option-to-vacuumdb.patch Description: Binary data
Re: ERROR: tuple concurrently updated when modifying privileges
On Tue, May 14, 2019 at 08:08:05AM +0200, Chris Travers wrote: > Having thought about this a bit, I think the best solution would be to have > grant take out an access share lock to the tables granted. This would > prevent concurrent alter table operations from altering the schema > underneath the grant as well, and thus possibly cause other race conditions. > > Any thoughts? "tuple concurrently updated" is an error message which should never be user-facing, and unfortunately there are many scenarios where it can be triggered by playing with concurrent DDLs: https://postgr.es/m/20171228063004.gb6...@paquier.xyz If you have an idea of patch, could you write it? Having an isolation test case would be nice as well. -- Michael signature.asc Description: PGP signature
can we create index/constraints in different schema
Hi, This is working in Oracle but not in postgresql 'CREATE INDEX client.test_1_idx ON dbo.test_1 (name);' . Can we implement this by another way? Thanks Navneet
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 06:58:14PM +1200, Thomas Munro wrote: > On Tue, May 14, 2019 at 6:18 PM Kyotaro HORIGUCHI > wrote: > > I played with this a bit and found that "... (attr=[tab]" (no > > space between "r" and "=") complets with '='. Isn't it annoying? > > > > Only "UPDATE hoge SET a=[tab]" behaves the same way among > > existing completions. > > Hmm. True. Here's one way to fix that. Hmm... just got here. What happens around here? > > -- > Thomas Munro > https://enterprisedb.com
Re: can we create index/constraints in different schema
On Tue, May 14, 2019 at 03:41:37AM -0400, navneet nikku wrote: >This is working in Oracle but not in postgresql 'CREATE INDEX > client.test_1_idx > ON dbo.test_1 (name);' . Can we implement this by another way? No, it is not possible to define a schema with CREATE INDEX, and an index gets located in the same schema as its depending table. -- Michael signature.asc Description: PGP signature
Re: can we create index/constraints in different schema
Ok, thanks for the clarification. Regards Navneet On Tue, May 14, 2019 at 4:33 AM Michael Paquier wrote: > On Tue, May 14, 2019 at 03:41:37AM -0400, navneet nikku wrote: > >This is working in Oracle but not in postgresql 'CREATE INDEX > > client.test_1_idx > > ON dbo.test_1 (name);' . Can we implement this by another way? > > No, it is not possible to define a schema with CREATE INDEX, and an > index gets located in the same schema as its depending table. > -- > Michael >
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 8:32 PM Edgy Hacker wrote: > Hmm... just got here. Welcome. > What happens around here? Please see https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F . -- Thomas Munro https://enterprisedb.com
Re: pg_stat_database update stats_reset only by pg_stat_reset
it reset statistics for a single table and update the column stats_reset of
pg_stat_database.
but i think that stats_reset shoud be database-level statistics, a single
table should not update the column stats_reset.
i am monitor the xact_commit every 5 minutes, when stats_reset is reset but
ohter columns is not reset, i can't decide
if i will recount the xact_commit, because pg_stat_reset make all column to
zero. pg_stat_reset_single_table_counters
only reset the column stats_reset.
张连壮 于2019年5月13日周一 下午3:30写道：
> pg_stat_reset_single_table_counters/pg_stat_reset_single_function_counters
> only update pg_stat_database column stats_reset.
> stat_reset shuld update when all the column is reset.
>
> sample:
> drop database if exists lzzhang_db;
> create database lzzhang_db;
> \c lzzhang_db
>
> create table lzzhang_tab(id int);
> insert into lzzhang_tab values(1);
> insert into lzzhang_tab values(1);
>
> select tup_fetched, stats_reset from pg_stat_database where
> datname='lzzhang_db';
> select pg_sleep(1);
>
> select pg_stat_reset_single_table_counters('lzzhang_tab'::regclass::oid);
> select tup_fetched, stats_reset from pg_stat_database where
> datname='lzzhang_db';
>
> result:
> tup_fetched | stats_reset
> -+---
> 514 | 2019-05-12 03:22:55.702753+08
> (1 row)
> tup_fetched | stats_reset
> -+---
> 710 | 2019-05-12 03:22:56.729336+08
> (1 row)
> tup_fetched is not reset but stats_reset is reset.
>
Re: Tab completion for CREATE TYPE
On Tue, May 14, 2019 at 09:01:27PM +1200, Thomas Munro wrote: > On Tue, May 14, 2019 at 8:32 PM Edgy Hacker wrote: > > Hmm... just got here. > > Welcome. Thanks. > > > What happens around here? > > Please see https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F > . Not exactly a prospective developer but if it ever comes it... > > -- > Thomas Munro > https://enterprisedb.com
Re: ERROR: tuple concurrently updated when modifying privileges
On Tue, May 14, 2019 at 9:11 AM Michael Paquier wrote: > On Tue, May 14, 2019 at 08:08:05AM +0200, Chris Travers wrote: > > Having thought about this a bit, I think the best solution would be to > have > > grant take out an access share lock to the tables granted. This would > > prevent concurrent alter table operations from altering the schema > > underneath the grant as well, and thus possibly cause other race > conditions. > > > > Any thoughts? > > "tuple concurrently updated" is an error message which should never be > user-facing, and unfortunately there are many scenarios where it can > be triggered by playing with concurrent DDLs: > https://postgr.es/m/20171228063004.gb6...@paquier.xyz > > If you have an idea of patch, could you write it? Having an isolation > test case would be nice as well. > I will give Nick a chance to do the patch if he wants it (I have reached out). Otherwise sure. I did notice one more particularly exotic corner case that is not resolved by this proposed fix. If you have two transactions with try to grant onto the same pg entity (table etc) *both* will typically fail on the same error. I am not sure that is a bad thing because I am not sure how concurrent grants are supposed to work with MVCC but I think that would require a fundamentally different approach. > -- > Michael > -- Best Regards, Chris Travers Head of Database Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com Saarbrücker Straße 37a, 10405 Berlin
Re: PG 12 draft release notes
On Sat, May 11, 2019, at 22:33, Bruce Momjian wrote: > http://momjian.us/pgsql_docs/release-12.html There is a typo in E.1.3.1.1.: > Expressions are evaluated at table partitioned table creation time. First "table" seems to be excessive. Regards, Nick.
Read-only access to temp tables for 2PC transactions
Hi, That is an attempt number N+1 to relax checks for a temporary table access in a transaction that is going to be prepared. One of the problems regarding the use of temporary tables in prepared transactions is that such transaction will hold locks for a temporary table after being prepared. That locks will prevent the backend from exiting since it will fail to acquire lock needed to delete temp table during exit. Also, re-acquiring such lock after server restart seems like an ill-defined operation. I tried to allow prepared transactions that opened a temporary relation only in AccessShare mode and then neither transfer this lock to a dummy PGPROC nor include it in a 'prepare' record. Such prepared transaction will not prevent the backend from exiting and can be committed from other backend or after a restart. However, that modification allows new DDL-related serialization anomaly: it will be possible to prepare transaction which read table A; then drop A; then commit the transaction. I not sure whether that is worse than not being able to access temp relations or not. On the other hand, it is possible to drop AccessShare locks only for temporary relation and don't change behavior for an ordinary table (in the attached patch this is done for all tables). Also, I slightly modified ON COMMIT DELETE code path. Right now all ON COMMIT DELETE temp tables are linked in a static list and if transaction accessed any temp table in any mode then during commit all tables from that list will be truncated. For a given patch that means that even if a transaction only did read from a temp table it anyway can access other temp tables with high lock mode during commit. I've added hashtable that tracks higher-than-AccessShare action with a temp table during current transaction, so during commit, only tables from that hash will be truncated. That way ON COMMIT DELETE tables in the backend will not prevent read-only access to some other table in a given backend. Any thoughts? -- Stas Kelvich Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 2PC-ro-temprels.patch Description: Binary data
Re: Failure in contrib test _int on loach
On 08/05/2019 01:31, Heikki Linnakangas wrote: On 02/05/2019 10:37, Heikki Linnakangas wrote: On 29/04/2019 16:16, Anastasia Lubennikova wrote: In previous emails, I have sent two patches with test and bugfix (see attached). After Heikki shared his concerns, I've rechecked the algorithm and haven't found any potential error. So, if other hackers are agreed with my reasoning, the suggested fix is sufficient and can be committed. I still believe there is a problem with grandparent splits with this. I'll try to construct a test case later this week, unless you manage to create one before that. Here you go. If you apply the two patches from https://www.postgresql.org/message-id/5d48ce28-34cf-9b03-5d42-dbd5457926bf%40postgrespro.ru, and run the attached script, it will print out something like this: postgres=# \i grandparent.sql DROP TABLE CREATE TABLE INSERT 0 15 CREATE INDEX psql:grandparent.sql:27: NOTICE: working on 1 psql:grandparent.sql:27: NOTICE: working on 2 psql:grandparent.sql:27: NOTICE: working on 3 psql:grandparent.sql:27: NOTICE: working on 4 psql:grandparent.sql:27: NOTICE: working on 5 psql:grandparent.sql:27: NOTICE: working on 6 psql:grandparent.sql:27: NOTICE: working on 7 psql:grandparent.sql:27: NOTICE: working on 8 psql:grandparent.sql:27: NOTICE: working on 9 psql:grandparent.sql:27: NOTICE: working on 10 psql:grandparent.sql:27: NOTICE: working on 11 psql:grandparent.sql:27: NOTICE: failed for 114034 psql:grandparent.sql:27: NOTICE: working on 12 DO That "failed for 114034" should not happen. Fortunately, that's not too hard to fix. We just need to arrange things so that the "retry_from_parent" flag also gets set for the grandparent, when the grandparent is split. Like in the attached patch. I hear no objections, so pushed that. But if you have a chance to review this later, just to double-check, I'd still appreciate that. - Heikki
Re: Tab completion for CREATE TYPE
At Tue, 14 May 2019 18:58:14 +1200, Thomas Munro wrote
in
> On Tue, May 14, 2019 at 6:18 PM Kyotaro HORIGUCHI
> wrote:
> > I played with this a bit and found that "... (attr=[tab]" (no
> > space between "r" and "=") complets with '='. Isn't it annoying?
> >
> > Only "UPDATE hoge SET a=[tab]" behaves the same way among
> > existing completions.
>
> Hmm. True. Here's one way to fix that.
Thanks. That's what was in my mind.
Some definition item names are induced from some current states
(e.g. "CREATE TYPE name AS RANGE (" => "SUBTYPE = ") but I think
it's too much.
COLLATE is not suggested with possible collations but I think
suggesting it is not so useful.
PASSEDBYVALUE is suggested with '=', which is different from
documented syntax but I don't think that's not such a problem for
those who spell this command out.
# By the way, collatable and preferred are boolean which behaves
# the same way with passedbyvalue. Is there any intention in the
# difference in the documentation?
The completion lists contain all possible words correctly (I
think "analyse" is an implicit synonym.).
As the result, I find it perfect.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Table AM callback table_complete_speculative()'s succeeded argument is reversed
The 'succeeded' argument seems backwards here:
static void
heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
uint32
spekToken, bool succeeded)
{
boolshouldFree = true;
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
/* adjust the tuple's state accordingly */
if (!succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);
if (shouldFree)
pfree(tuple);
}
According to the comments, if "succeeded = true", the insertion is
completed, and otherwise it's killed. It works, because the only caller
is also passing the argument wrong.
Barring objections, I'll push the attached patch to fix that.
- Heikki
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bc47856ad5..00505ec3f4 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -282,7 +282,7 @@ heapam_tuple_complete_speculative(Relation relation, TupleTableSlot *slot,
HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
/* adjust the tuple's state accordingly */
- if (!succeeded)
+ if (succeeded)
heap_finish_speculative(relation, &slot->tts_tid);
else
heap_abort_speculative(relation, &slot->tts_tid);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 444c0c0574..d545bbce8a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -556,7 +556,7 @@ ExecInsert(ModifyTableState *mtstate,
/* adjust the tuple's state accordingly */
table_complete_speculative(resultRelationDesc, slot,
- specToken, specConflict);
+ specToken, !specConflict);
/*
* Wake up anyone waiting for our decision. They will re-check
RE: Copy data to DSA area
Hi,
>>From: Thomas Munro [mailto:thomas.mu...@gmail.com]
>>Subject: Re: Copy data to DSA area
>>
>>On Wed, May 8, 2019 at 5:29 PM Ideriha, Takeshi
>>
>>wrote:
>>> >From: Ideriha, Takeshi [mailto:ideriha.take...@jp.fujitsu.com]
>>> >Sent: Friday, April 26, 2019 11:50 PM Well, after developing PoC, I
>>> >realized that this PoC doesn't solve the local process is crashed
>>> >before the context becomes shared because local process keeps track
>>> >of pointer to chunks.
>>> >Maybe all of you have already noticed and pointed out this case :)
>>> >So it needs another work but this poc is a good step for me to advance
>>> >more.
>>>
>>> I think the point to prevent memory leak is allocating memory and
>>> storing its address into a structure at the same time. This structure
>>> should be trackable from other process.
>>
>>I'm not sure that it's necessarily wrong to keep tracking information in
>>private
>memory.
>>If any backend crashes, the postmaster will terminate all backends and
>>reinitialise everything anyway, because shared memory might be corrupted.
>
>I'm going to put keep tracking information in private
>memory and send a patch.
I updated a PoC patch.
This has memory tracking buffer in local process. The old version also has this
system but I refactored the code. Memory leak while allocating memory seems to
be solved thanks to memory tracking buffer.
What I haven't addressed is memory leak while freeing objects. In current
sequence a cache (e.g. relcache) is freed after removed from its hash table.
If cache and hash table gets shared, memory leak is likely to happen
between removal from hash table and free. We lose track of cache objects
if error happens after cache is unlinked from the hash table. And also a cache
consists of graph structure. So we also take care of freeing cache partially.
Maybe we need to remember pointers of objects before unlink from the hash.
Also, we need to free them all at once after we can make sure that all the
pointers are registered to local buffer. Followings are some idea to implement
this:
- change the order of removal from hash table and deletion
- pfree in shared memory context doesn't dsa_free but just add pointer to
the local buffer.
- remove link from hash table after all pfree() is done
- then, call a function, which does actual dsa_free taking a look at the local
Buffer
But I'm not sure this solution is good one. Do you have any thoughts?
Regards,
Takeshi Ideriha
/*
* Another PoC of MemoryContext for DSA based on Thomus Munro's work
*
* https://www.postgresql.org/message-id/
* CAEepm%3D22H0TDCAHWh%3DYWmSV%2BX%2BbTtcTNg8RpP%3DeaCjWJU_d-9A
* %40mail.gmail.com
*
*/
#include "postgres.h"
#include "fmgr.h"
#include "lib/ilist.h"
#include "miscadmin.h"
#include "nodes/pg_list.h"
#include "nodes/memnodes.h"
#include "storage/ipc.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "utils/dsa.h"
#include "utils/memutils.h"
#define MY_AREA_SIZE (1024 * 1024)
#define NUM_CHUNKS 64 /* an arbitrary number */
PG_MODULE_MAGIC;
void _PG_init(void);
PG_FUNCTION_INFO_V1(hoge);
PG_FUNCTION_INFO_V1(hoge_list);
PG_FUNCTION_INFO_V1(hoge_list_error);
#define isLocalContext(c) (c->alloc_buffer != NULL)
#define isBufferFull(buf) (buf->tail == NUM_CHUNKS)
/* Support code to make a dsa_area into a MemoryContext. */
typedef struct dsa_temp_buffer dsa_temp_buffer;
static void hoge_shmem_startup_hook(void);
static MemoryContext make_hoge_memory_context(dsa_area *area, void *base, bool
isLocal);
static void push_new_buffer(dsa_temp_buffer *buffer);
static shmem_startup_hook_type prev_shmem_startup_hook;
static void *my_raw_memory;
static dsa_area *my_area;
static MemoryContext my_shared_dsa_context;
static MemoryContext my_local_dsa_context;
static List **my_list;
void AddToDsaSharedContext(MemoryContext local, MemoryContext shared);
MemoryContext CreateDsaLocalContext(MemoryContext shared);
Size ShmDsaContextSize(void);
void
_PG_init(void)
{
/* This only works if preloaded by the postmaster. */
if (!process_shared_preload_libraries_in_progress)
return;
/* Request a chunk of traditional shared memory. */
RequestAddinShmemSpace(MY_AREA_SIZE);
/* Register our hook for phase II of initialization. */
prev_shmem_startup_hook = shmem_startup_hook;
shmem_startup_hook = hoge_shmem_startup_hook;
}
static void
hoge_shmem_startup_hook(void)
{
MemoryContext old_context;
boolfound;
if (prev_shmem_startup_hook)
prev_shmem_startup_hook();
old_context = MemoryContextSwitchTo(TopMemoryContext);
LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
/* Allocate, or look up, a chunk of raw fixed-address shared memory. */
my_raw_memory = ShmemInitStruct("hoge", MY_AREA_SIZE, &found);
if (!found)
{
/*
* Create a new DSA area, and clamp its size so it can't
Re: How to install login_hook in Postgres 10.5
On 5/13/19 8:32 PM, Michael Paquier wrote: > On Mon, May 13, 2019 at 01:06:10PM -0700, legrand legrand wrote: >> that finished commited >> "pgsql: Add hooks for session start and session end" >> https://www.postgresql.org/message-id/flat/575d6fa2-78d0-4456-8600-302fc35b2591%40dunslane.net#0819e315c6e44c49a36c69080cab644d >> >> but was finally rollbacked because it didn't pass installcheck test >> ... >> >> Maybe you are able to patch your pg installation, >> it would be a solution of choice (there is a nice exemple >> of extension included) > > You will need to patch Postgres to add this hook, and you could > basically reuse the patch which has been committed once. I don't > think that it would be that much amount of work to get it working > correctly on the test side to be honest, so we may be able to get > something into v13 at this stage. This is mainly a matter of > resources though, and of folks willing to actually push for it. I am interested in this, so if Andrew wants to create a buildfarm module I will either add it to rhinoceros or stand up another buildfarm animal for it. I am also happy to help push it for v13. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: PG 12 draft release notes
On Tue, May 14, 2019 at 11:53:23AM +0200, nickb wrote: > On Sat, May 11, 2019, at 22:33, Bruce Momjian wrote: > > http://momjian.us/pgsql_docs/release-12.html > > There is a typo in E.1.3.1.1.: > > Expressions are evaluated at table partitioned table creation time. > First "table" seems to be excessive. Yep, fixed, thanks. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Table AM callback table_complete_speculative()'s succeeded argument is reversed
Hi,
On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas wrote:
>The 'succeeded' argument seems backwards here:
>
>> static void
>> heapam_tuple_complete_speculative(Relation relation, TupleTableSlot
>*slot,
>>uint32
>> spekToken, bool succeeded)
>> {
>> boolshouldFree = true;
>> HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
>>
>> /* adjust the tuple's state accordingly */
>> if (!succeeded)
>> heap_finish_speculative(relation, &slot->tts_tid);
>> else
>> heap_abort_speculative(relation, &slot->tts_tid);
>>
>> if (shouldFree)
>> pfree(tuple);
>> }
>
>According to the comments, if "succeeded = true", the insertion is
>completed, and otherwise it's killed. It works, because the only caller
>
>is also passing the argument wrong.
Thanks for finding.
>Barring objections, I'll push the attached patch to fix that.
Please hold off - your colleagues found this before, and I worked on getting
test coverage for the code. It's scheduled for commit together today.
Unfortunately nobody looked at the test much...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [HACKERS] advanced partition matching algorithm for partition-wise join
On Tue, May 14, 2019 at 10:00 AM Amit Langote wrote: > On 2019/05/14 13:23, Amit Langote wrote: > > Tom > > strongly objected to that idea saying that such join paths are kind of > > silly [1], even outside the context of partitionwise join. He suggested > > that we abandon partitionwise join in such cases, because having to build > > a dummy base relation for pruned partitions only to generate > silly-looking > > paths would be an ugly kludge. > > I forgot to mention that he even committed a patch to disable > partitionwise joins in such cases, which was also applied to v11 branch. > > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d70c147fa217c4bae32ac1afb86ab42d98b36fdf > > Note that there were also other reasons for committing, beside what I > described in my previous email. > > I haven't seen the actual commit, but we could use these patches to enable partition-wise join when partitions are pruned. For that the partition descriptor of the pruned partition table should be arranged as if those partitions are missing in the table itself. However, we will still need code to handle the cases when the partitions are missing on the nullable side. Tom mentioned the idea of using just projection to produce join tuples with rows on the outer side appended with null columns from the nullable side. If we can implement that, we can remove the restrictions in this patch. -- Best Wishes, Ashutosh Bapat
Re: PANIC :Call AbortTransaction when transaction id is no normal
Andres Freund writes: > On 2019-05-14 12:37:39 +0900, Michael Paquier wrote: >> Still, I would like to understand why the bootstrap process has been >> signaled to begin with, particularly for an initdb, which is not >> really something that should happen on a server where an instance >> runs. If you have a too aggressive monitoring job, you may want to >> revisit that as well, because it is able to complain just with an >> initdb. > Shutdown, timeout, resource exhaustion all seem like possible > causes. Don't think any of them warrant a core file - as the OP > explains, that'll often trigger pages etc. Yeah. The case I was thinking about was mostly "start initdb, decide I didn't want to do that, hit control-C". That cleans up without much fuss *except* if you manage to hit the window where it's running bootstrap, and then it spews this scary-looking error. It's less scary-looking with the SIG_DFL patch, which I've now pushed. regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 03, 2019 at 11:45:33AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-03 10:08:59 -0700, Mark Wong wrote: > > Ok, I think I have gdb installed now... > > Thanks! Any chance you could turn on force_parallel_mode for the other > branches it applies to too? Makes it easier to figure out whether > breakage is related to that, or independent. Slowly catching up on my collection of ppc64le animals... I still need to upgrade the build farm client (v8) on: * dhole * vulpes * wobbegong * cuon * batfish * devario * cardinalfish The following I've enabled force_parallel_mode for HEAD, 11, 10, and 9.6: * buri * urocryon * ayu * shoveler * chimaera * bonito * takin * bufflehead * elasmobranch * demoiselle * cavefish Regards, Mark
Re: Why is infinite_recurse test suddenly failing?
Mark Wong writes: > The following I've enabled force_parallel_mode for HEAD, 11, 10, and > 9.6: Thanks Mark! In theory, the stack trace we now have from shoveler proves that parallel mode has nothing to do with this, because the crash happens during planning (specifically, inlining SQL functions). I wonder though if it's possible that previous force_parallel_mode queries have had some undesirable effect on the process's stack allocation. Just grasping at straws, because it's sure not clear what's going wrong. regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
Mark Wong writes: > Slowly catching up on my collection of ppc64le animals... Oh, btw ... you didn't answer my question from before: are these animals all running on a common platform (and if so, what is that), or are they really different hardware? If they're all VMs on one machine then it might be that there's some common-mode effect from the underlying system. (Another thing I notice, now, is that these are all Linux variants; I'd been thinking you had some BSDen in there too, but now I see that none of those are ppc64. Hm.) regards, tom lane
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 10, 2019 at 11:27:07AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-10 11:38:57 -0400, Tom Lane wrote: > > Core was generated by `postgres: debian regression [local] SELECT > > '. > > Program terminated with signal SIGSEGV, Segmentation fault. > > #0 sysmalloc (nb=8208, av=0x3fff916e0d28 ) at malloc.c:2748 > > 2748malloc.c: No such file or directory. > > #0 sysmalloc (nb=8208, av=0x3fff916e0d28 ) at malloc.c:2748 > > #1 0x3fff915bedc8 in _int_malloc (av=0x3fff916e0d28 , > > bytes=8192) at malloc.c:3865 > > #2 0x3fff915c1064 in __GI___libc_malloc (bytes=8192) at malloc.c:2928 > > #3 0x106acfd8 in AllocSetContextCreateInternal > > (parent=0x1000babdad0, name=0x1085508c "inline_function", > > minContextSize=, initBlockSize=, > > maxBlockSize=8388608) at aset.c:477 > > #4 0x103d5e00 in inline_function (funcid=20170, > > result_type=, result_collid=, > > input_collid=, funcvariadic=, > > func_tuple=, context=0x3fffe3da15d0, args=) > > at clauses.c:4459 > > #5 simplify_function (funcid=, result_type=, > > result_typmod=, result_collid=, > > input_collid=, args_p=, > > funcvariadic=, process_args=, > > allow_non_const=, context=) at clauses.c:4040 > > #6 0x103d2e74 in eval_const_expressions_mutator > > (node=0x1000babe968, context=0x3fffe3da15d0) at clauses.c:2474 > > #7 0x103511bc in expression_tree_mutator (node=, > > mutator=0x103d2b10 , > > context=0x3fffe3da15d0) at nodeFuncs.c:2893 > > > > So that lets out any theory that somehow we're getting into a weird > > control path that misses calling check_stack_depth; > > expression_tree_mutator does so for one, and it was called just nine > > stack frames down from the crash. > > Right. There's plenty places checking it... > > > > I am wondering if, somehow, the stack depth limit seen by the postmaster > > sometimes doesn't apply to its children. That would be pretty wacko > > kernel behavior, especially if it's only intermittently true. > > But we're running out of other explanations. > > I wonder if this is a SIGSEGV that actually signals an OOM > situation. Linux, if it can't actually extend the stack on-demand due to > OOM, sends a SIGSEGV. The signal has that information, but > unfortunately the buildfarm code doesn't print it. p $_siginfo would > show us some of that... > > Mark, how tight is the memory on that machine? There's about 2GB allocated: debian@postgresql-debian:~$ cat /proc/meminfo MemTotal:2080704 kB MemFree: 1344768 kB MemAvailable:1824192 kB At the moment it looks like plenty. :) Maybe I should set something up to monitor these things. > Does dmesg have any other > information (often segfaults are logged by the kernel with the code > IIRC). It's been up for about 49 days: debian@postgresql-debian:~$ uptime 14:54:30 up 49 days, 14:59, 3 users, load average: 0.00, 0.34, 1.04 I see one line from dmesg that is related to postgres: [3939350.616849] postgres[17057]: bad frame in setup_rt_frame: 3fffe3d9fe00 nip 3fff915bdba0 lr 3fff915bde9c But only that one time in 49 days up. Otherwise I see a half dozen hung_task_timeout_secs messages around jdb2 and dhclient. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Re: Why is infinite_recurse test suddenly failing?
On Tue, May 14, 2019 at 10:52:07AM -0400, Tom Lane wrote: > Mark Wong writes: > > Slowly catching up on my collection of ppc64le animals... > > Oh, btw ... you didn't answer my question from before: are these animals > all running on a common platform (and if so, what is that), or are they > really different hardware? If they're all VMs on one machine then it > might be that there's some common-mode effect from the underlying system. Sorry, I was almost there. :) These systems are provisioned with OpenStack. Additionally, a couple more (cardinalfish, devario) are using docker under that. > (Another thing I notice, now, is that these are all Linux variants; > I'd been thinking you had some BSDen in there too, but now I see > that none of those are ppc64. Hm.) Right, the BSDen I have are on different hardware. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Re: Why is infinite_recurse test suddenly failing?
On Fri, May 10, 2019 at 05:26:43PM -0400, Andrew Dunstan wrote: > > On 5/10/19 3:35 PM, Tom Lane wrote: > > Andres Freund writes: > >> On 2019-05-10 11:38:57 -0400, Tom Lane wrote: > >>> I am wondering if, somehow, the stack depth limit seen by the postmaster > >>> sometimes doesn't apply to its children. That would be pretty wacko > >>> kernel behavior, especially if it's only intermittently true. > >>> But we're running out of other explanations. > >> I wonder if this is a SIGSEGV that actually signals an OOM > >> situation. Linux, if it can't actually extend the stack on-demand due to > >> OOM, sends a SIGSEGV. The signal has that information, but > >> unfortunately the buildfarm code doesn't print it. p $_siginfo would > >> show us some of that... > >> Mark, how tight is the memory on that machine? Does dmesg have any other > >> information (often segfaults are logged by the kernel with the code > >> IIRC). > > It does sort of smell like a resource exhaustion problem, especially > > if all these buildfarm animals are VMs running on the same underlying > > platform. But why would that manifest as "you can't have a measly two > > megabytes of stack" and not as any other sort of OOM symptom? > > > > Mark, if you don't mind modding your local copies of the buildfarm > > script, I think what Andres is asking for is a pretty trivial addition > > in PGBuild/Utils.pm's sub get_stack_trace: > > > > my $cmdfile = "./gdbcmd"; > > my $handle; > > open($handle, '>', $cmdfile) || die "opening $cmdfile: $!"; > > print $handle "bt\n"; > > + print $handle "p $_siginfo\n"; > > close($handle); > > > > > > > I think we'll need to write that as: > > > print $handle 'p $_siginfo',"\n"; Ok, I have this added to everyone now. I think I also have caught up on this thread, but let me know if I missed anything. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
SEASON OF DOCS PROJECT
Hi there I am interested about the project and have gone through the project idea. But I would like to know more about the project and the organization expectations the tech writers .Apart from the skills and language mentioned..what more skills/language you are expecting from technical writers. please let me know.
Re: Inconsistent error message wording for REINDEX CONCURRENTLY
On 2019-May-09, Michael Paquier wrote:
> On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
> > Michael Paquier writes:
> >> No problem to do that. I'll brush up all that once you commit the
> >> first piece you have come up with, and reuse the new API of catalog.c
> >> you are introducing based on the table OID.
> >
> > Pushed my stuff, have at it.
>
> Thanks. Attached is what I get to after scanning the error messages
> in indexcmds.c and index.c. Perhaps you have more comments about it?
I do :-) There are a couple of "is not supported" messages that are
annoyingly similar but different:
git grep --show-function 'reindex.*supported' -- *.c
src/backend/commands/indexcmds.c=ReindexMultipleTables(const char *objectName,
ReindexObjectType objectKind,
src/backend/commands/indexcmds.c:errmsg("concurrent reindex of
system catalogs is not supported")));
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for catalog relations, skipping all")));
src/backend/commands/indexcmds.c=ReindexRelationConcurrently(Oid relationOid,
int options)
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for shared relations")));
src/backend/commands/indexcmds.c:errmsg("concurrent
reindex is not supported for catalog relations")));
It seems strange to have some cases say "cannot do foo" and other cases
say "foo is not supported". However, I think having
ReindexMultipleTables say "cannot reindex a system catalog" would be
slightly wrong (since we're not reindexing one but many) -- so it would
have to be "cannot reindex system catalogs". And in order to avoid
having two messages that are essentially identical except in number, I
propose to change the others to use the plural too. So the one you just
committed
> + /* A system catalog cannot be reindexed
> concurrently */
> + if (IsCatalogRelationOid(relationOid))
> + ereport(ERROR,
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot reindex
> a system catalog concurrently")));
would become "cannot reindex system catalogs concurrently", identical to
the one in ReindexMultipleTables.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: SQL-spec incompatibilities in similar_escape() and related stuff
[ backing up to a different sub-discussion ]
Andrew Gierth writes:
> "Tom" == Tom Lane writes:
> Tom> To support the first usage, similar_escape is non-strict, and it
> Tom> takes a NULL second argument to mean '\'. This is already a SQL
> Tom> spec violation, because as far as I can tell from the spec, if you
> Tom> don't write an ESCAPE clause then there is *no* escape character;
> Tom> there certainly is not a default of '\'. However, we document this
> Tom> behavior, so I don't know if we want to change it.
> This is the same spec violation that we also have for LIKE, which also
> is supposed to have no escape character in the absense of an explicit
> ESCAPE clause.
Right. After further thought, I propose that what we ought to do is
unify LIKE, SIMILAR TO, and 3-arg SUBSTRING on a single set of behaviors
for the ESCAPE argument:
1. They are strict, ie a NULL value for the escape string produces a
NULL result. This is per spec, and we don't document anything different,
and nobody would really expect something different. (But see below
about keeping similar_escape() as a legacy compatibility function.)
2. Omitting the ESCAPE option (not possible for SUBSTRING) results in a
default of '\'. This is not per spec, but we've long documented it this
way, and frankly I'd say that it's a far more useful default than the
spec's behavior of "there is no escape character". I propose that we
just document that this is not-per-spec and move on.
3. Interpret an empty ESCAPE string as meaning "there is no escape
character". This is not per spec either (the spec would have us
throw an error) but it's our historical behavior, and it seems like
a saner approach than the way the spec wants to do it --- in particular,
there's no way to get that behavior in 3-arg SUBSTRING if we don't allow
this.
So only point 1 represents an actual behavioral change from what we've
been doing; the other two just require doc clarifications.
Now, I don't have any problem with changing what happens when somebody
actually writes "a LIKE b ESCAPE NULL"; it seems fairly unlikely that
anyone would expect that to yield a non-null result. However, we do
have a problem with the fact that the implementation is partially
exposed:
regression=# create view v1 as select f1 similar to 'x*' from text_tbl;
CREATE VIEW
regression=# \d+ v1
...
View definition:
SELECT text_tbl.f1 ~ similar_escape('x*'::text, NULL::text)
FROM text_tbl;
If we just change similar_escape() to be strict, then this view will
stop working, which is a bit hard on users who did not write anything
non-spec-compliant.
I propose therefore that we leave similar_escape in place with its
current behavior, as a compatibility measure for cases like this.
Intead, invent two new strict functions, say
similar_to_escape(pattern)
similar_to_escape(pattern, escape)
and change the parser and the implementation of SUBSTRING() to
rely on these going forward.
The net effect will be to make explicit "ESCAPE NULL" spec-compliant,
and to get rid of the performance problem from inlining failure for
substring(). All else is just doc clarifications.
Comments?
regards, tom lane
VACUUM fails to parse 0 and 1 as boolean value
Hi, VACUUM fails to parse 0 and 1 as boolean value The document for VACUUM explains boolean Specifies whether the selected option should be turned on or off. You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF, or 0 to disable it. But VACUUM fails to parse 0 and 1 as boolean value as follows. =# VACUUM (INDEX_CLEANUP 1); ERROR: syntax error at or near "1" at character 23 STATEMENT: VACUUM (INDEX_CLEANUP 1); This looks a bug. The cause of this is a lack of NumericOnly clause for vac_analyze_option_arg in gram.y. The attached patch adds such NumericOnly. The bug exists only in 12dev. Barring any objection, I will commit the patch. Regards, -- Fujii Masao vacuum_numeric_as_boolean.patch Description: Binary data
Re: VACUUM fails to parse 0 and 1 as boolean value
Hi, On 2019-05-15 02:45:21 +0900, Fujii Masao wrote: > VACUUM fails to parse 0 and 1 as boolean value > > The document for VACUUM explains > > boolean > Specifies whether the selected option should be turned on or off. > You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF, > or 0 to disable it. > > But VACUUM fails to parse 0 and 1 as boolean value as follows. > > =# VACUUM (INDEX_CLEANUP 1); > ERROR: syntax error at or near "1" at character 23 > STATEMENT: VACUUM (INDEX_CLEANUP 1); > > This looks a bug. The cause of this is a lack of NumericOnly clause > for vac_analyze_option_arg in gram.y. The attached patch > adds such NumericOnly. The bug exists only in 12dev. > > Barring any objection, I will commit the patch. Might be worth having a common rule for such options, so we don't duplicate the knowledge between different places. CCing Robert and Sawada-san, who committed / authored that code. commit 41b54ba78e8c4d64679ba4daf82e4e2efefe1922 Author: Robert Haas Date: 2019-03-29 08:22:49 -0400 Allow existing VACUUM options to take a Boolean argument. This makes VACUUM work more like EXPLAIN already does without changing the meaning of any commands that already work. It is intended to facilitate the addition of future VACUUM options that may take non-Boolean parameters or that default to false. Masahiko Sawada, reviewed by me. Discussion: http://postgr.es/m/CA+TgmobpYrXr5sUaEe_T0boabV0DSm=utsozzwcunqfleem...@mail.gmail.com Discussion: http://postgr.es/m/CAD21AoBaFcKBAeL5_++j+Vzir2vBBcF4juW7qH8b3HsQY=q...@mail.gmail.com Greetings, Andres Freund
Re: vacuumdb and new VACUUM options
On Tue, May 14, 2019 at 10:01 AM Michael Paquier wrote: > > On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote: > > Thank you! I've incorporated your comment in my branch. I'll post the > > updated version patch after the above discussion got a consensus. > > Fujii-san, any input about the way to move forward here? Beta1 is > planned for next week, hence it would be nice to progress on this > front this week. I think that we can push "--index-cleanup=BOOLEAN" version into beta1, and then change the interface of the options if we received many complaints about "--index-cleanup=BOOLEAN" from users. So this week, I'd like to review Sawada's patch and commit it if that's ok. Regards, -- Fujii Masao
Re: vacuumdb and new VACUUM options
On Thu, May 9, 2019 at 8:20 PM Masahiko Sawada wrote: > > On Thu, May 9, 2019 at 10:01 AM Michael Paquier wrote: > > > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote: > > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao > > > escreveu: > > >> The question is; we should support vacuumdb option for (1), i.e.,, > > >> something like --index-cleanup option is added? > > >> Or for (2), i.e., something like --disable-index-cleanup option is added > > >> as your patch does? Or for both? > > > > > > --index-cleanup=BOOL > > > > I agree with Euler's suggestion to have a 1-1 mapping between the > > option of vacuumdb and the VACUUM parameter > > +1. Attached the draft version patches for both options. Thanks for the patch! + if (strncasecmp(opt_str, "true", 4) != 0 && + strncasecmp(opt_str, "false", 5) != 0) Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, like VACUUM does? + char *index_cleanup; The patch would be simpler if enum trivalue is used for index_cleanup variable as the type. Regards, -- Fujii Masao
Re: PG12, PGXS and linking pgfeutils
I wrote: > I think moving fe_utils/logging.[hc] to > common/ is definitely the way to get out of this problem. I've pushed that, so Ian's problem should be gone as of HEAD. regards, tom lane
Re: Inconsistency between table am callback and table function names
On Mon, May 13, 2019 at 12:51 PM Robert Haas wrote: > On Fri, May 10, 2019 at 3:43 PM Ashwin Agrawal > wrote: > > Meant to stick the question mark in that email, somehow missed. Yes > > not planning to spend any time on it if objections. Here is the list > > of renames I wish to perform. > > > > Lets start with low hanging ones. > > > > table_rescan -> table_scan_rescan > > table_insert -> table_tuple_insert > > table_insert_speculative -> table_tuple_insert_speculative > > table_delete -> table_tuple_delete > > table_update -> table_tuple_update > > table_lock_tuple -> table_tuple_lock > > > > Below two you already mentioned no objections to rename > > table_fetch_row_version -> table_tuple_fetch_row_version > > table_get_latest_tid -> table_tuple_get_latest_tid > > > > Now, table_beginscan and table_endscan are the ones which are > > wide-spread. > > I vote to rename all the ones where the new name would contain "tuple" > and to leave the others alone. i.e. leave table_beginscan, > table_endscan, and table_rescan as they are. I think that there's > little benefit in standardizing table_rescan but not the other two, > and we seem to agree that tinkering with the other two gets into a > painful amount of churn. > Thank you. Please find the patch to rename the agreed functions. It would be good to make all consistent instead of applying exception to three functions but seems no consensus on it. Given table_ api are new, we could modify them leaving systable_ ones as is, but as objections left that as is. From 39fcc223a0aaeacf545e09cfe29b8d565d970234 Mon Sep 17 00:00:00 2001 From: Ashwin Agrawal Date: Tue, 14 May 2019 10:10:21 -0700 Subject: [PATCH] Rename table am wrappers to match table am callback names. Now all the table wrapper functions match the "table_" theme, except table_beginscan(), table_endscan() and table_rescan(). The exception was applied and decision is not to rename the three functions based on discussion to avoid code churn. --- src/backend/access/heap/heapam.c | 10 ++-- src/backend/access/table/tableam.c | 46 +++ src/backend/commands/copy.c| 12 ++-- src/backend/commands/createas.c| 14 ++--- src/backend/commands/matview.c | 14 ++--- src/backend/commands/tablecmds.c | 4 +- src/backend/commands/trigger.c | 12 ++-- src/backend/executor/execMain.c| 8 +-- src/backend/executor/execReplication.c | 16 +++--- src/backend/executor/nodeLockRows.c| 8 +-- src/backend/executor/nodeModifyTable.c | 78 - src/backend/executor/nodeTidscan.c | 4 +- src/backend/utils/adt/tid.c| 4 +- src/include/access/tableam.h | 80 +- 14 files changed, 155 insertions(+), 155 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index ec9853603fd..86a964c4f48 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1856,12 +1856,12 @@ ReleaseBulkInsertStatePin(BulkInsertState bistate) * The new tuple is stamped with current transaction ID and the specified * command ID. * - * See table_insert for comments about most of the input flags, except that + * See table_tuple_insert for comments about most of the input flags, except that * this routine directly takes a tuple rather than a slot. * * There's corresponding HEAP_INSERT_ options to all the TABLE_INSERT_ * options, and there additionally is HEAP_INSERT_SPECULATIVE which is used to - * implement table_insert_speculative(). + * implement table_tuple_insert_speculative(). * * On return the header fields of *tup are updated to match the stored tuple; * in particular tup->t_self receives the actual TID where the tuple was @@ -2434,7 +2434,7 @@ xmax_infomask_changed(uint16 new_infomask, uint16 old_infomask) /* * heap_delete - delete a tuple * - * See table_delete() for an explanation of the parameters, except that this + * See table_tuple_delete() for an explanation of the parameters, except that this * routine directly takes a tuple rather than a slot. * * In the failure cases, the routine fills *tmfd with the tuple's t_ctid, @@ -2880,7 +2880,7 @@ simple_heap_delete(Relation relation, ItemPointer tid) /* * heap_update - replace a tuple * - * See table_update() for an explanation of the parameters, except that this + * See table_tuple_update() for an explanation of the parameters, except that this * routine directly takes a tuple rather than a slot. * * In the failure cases, the routine fills *tmfd with the tuple's t_ctid, @@ -3951,7 +3951,7 @@ get_mxact_status_for_lock(LockTupleMode mode, bool is_update) * *buffer: set to buffer holding tuple (pinned but not locked at exit) * *tmfd: filled in failure cases (see below) * - * Function results are the same as the ones for table_lock_tuple(). + * Function results are the same as the ones for table_tuple_lock(). * * In the failure cases
Re: Inconsistency between table am callback and table function names
On Wed, May 8, 2019 at 5:05 PM Ashwin Agrawal wrote: > Not having consistency is the main aspect I wish to bring to > attention. Like for some callback functions the comment is > > /* see table_insert() for reference about parameters */ > void(*tuple_insert) (Relation rel, TupleTableSlot *slot, > CommandId cid, int options, > struct BulkInsertStateData *bistate); > > /* see table_insert_speculative() for reference about parameters > */ > void(*tuple_insert_speculative) (Relation rel, > TupleTableSlot *slot, > CommandId cid, > int options, > struct > BulkInsertStateData *bistate, > uint32 specToken); > > Whereas for some other callbacks the parameter explanation exist in > both the places. Seems we should be consistent. > I feel in long run becomes pain to keep them in sync as comments > evolve. Like for example > > /* > * Estimate the size of shared memory needed for a parallel scan > of this > * relation. The snapshot does not need to be accounted for. > */ > Size(*parallelscan_estimate) (Relation rel); > > parallescan_estimate is not having snapshot argument passed to it, but > table_parallescan_estimate does. So, this way chances are high they > going out of sync and missing to modify in both the places. Agree > though on audience being different for both. So, seems going with the > refer XXX for parameters seems fine approach for all the callbacks and > then only specific things to flag out for the AM layer to be aware can > live above the callback function. > The topic of consistency for comment style for all wrappers seems got lost with other discussion, would like to seek opinion on the same.
Re: Adding a test for speculative insert abort case
Hi, On 2019-05-10 14:40:38 -0700, Andres Freund wrote: > On 2019-05-01 11:41:48 -0700, Andres Freund wrote: > > I'm imagining something like > > > > if (pg_try_advisory_xact_lock(1)) > > pg_advisory_xact_lock(2); > > else > > pg_advisory_xact_lock(1); > > > > in t1_lock_func. If you then make the session something roughly like > > > > s1: pg_advisory_xact_lock(1); > > s1: pg_advisory_xact_lock(2); > > > > s2: upsert t1 > > s1: pg_advisory_xact_unlock(1); > > s2: > > s2: > > s1: insert into t1 values(1, 'someval'); > > s1: pg_advisory_xact_unlock(2); > > s2: > > s2: spec-conflict > > Needed to be slightly more complicated than that, but not that much. See > the attached test. What do you think? > > I think we should apply something like this (minus the WARNING, of > course). It's a bit complicated, but it seems worth covering this > special case. And pushed. Let's see what the buildfarm says. Regards, Andres
Re: Match table_complete_speculative() code to comment
Hi,
On 2019-04-30 11:53:38 -0700, Ashwin Agrawal wrote:
> Comment for table_complete_speculative() says
>
> /*
> * Complete "speculative insertion" started in the same transaction.
> If
> * succeeded is true, the tuple is fully inserted, if false, it's
> removed.
> */
> static inline void
> table_complete_speculative(Relation rel, TupleTableSlot *slot,
>uint32 specToken, bool succeeded)
> {
> rel->rd_tableam->tuple_complete_speculative(rel, slot, specToken,
> succeeded);
> }
>
> but code really refers to succeeded as failure. Since that argument is
> passed as specConflict, means conflict happened and hence the tuple
> should be removed. It would be better to fix the code to match the
> comment as in AM layer its better to deal with succeeded to finish the
> insertion and not other way round.
>
> diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 4d179881f27..241639cfc20 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -282,7 +282,7 @@ heapam_tuple_complete_speculative(Relation
> relation, TupleTableSlot *slot,
> HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true,
> &shouldFree);
>
> /* adjust the tuple's state accordingly */
> - if (!succeeded)
> + if (succeeded)
> heap_finish_speculative(relation, &slot->tts_tid);
> else
> heap_abort_speculative(relation, &slot->tts_tid);
> diff --git a/src/backend/executor/nodeModifyTable.c
> b/src/backend/executor/nodeModifyTable.c
> index 444c0c05746..d545bbce8a2 100644
> --- a/src/backend/executor/nodeModifyTable.c
> +++ b/src/backend/executor/nodeModifyTable.c
> @@ -556,7 +556,7 @@ ExecInsert(ModifyTableState *mtstate,
>
> /* adjust the tuple's state accordingly */
> table_complete_speculative(resultRelationDesc, slot,
> -
> specToken, specConflict);
> +
> specToken, !specConflict);
And pushed, as
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=aa4b8c61d2cd57b53be03defb04d59b232a0e150
with the part that wasn't covered by tests now covered by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=08e2edc0767ab6e619970f165cb34d4673105f23
Greetings,
Andres Freund
Re: Table AM callback table_complete_speculative()'s succeeded argument is reversed
Hi,
On 2019-05-14 07:06:34 -0700, Andres Freund wrote:
> On May 14, 2019 4:29:01 AM PDT, Heikki Linnakangas wrote:
> >The 'succeeded' argument seems backwards here:
> >
> >> static void
> >> heapam_tuple_complete_speculative(Relation relation, TupleTableSlot
> >*slot,
> >> uint32
> >> spekToken, bool succeeded)
> >> {
> >>boolshouldFree = true;
> >>HeapTuple tuple = ExecFetchSlotHeapTuple(slot, true, &shouldFree);
> >>
> >>/* adjust the tuple's state accordingly */
> >>if (!succeeded)
> >>heap_finish_speculative(relation, &slot->tts_tid);
> >>else
> >>heap_abort_speculative(relation, &slot->tts_tid);
> >>
> >>if (shouldFree)
> >>pfree(tuple);
> >> }
> >
> >According to the comments, if "succeeded = true", the insertion is
> >completed, and otherwise it's killed. It works, because the only caller
> >
> >is also passing the argument wrong.
>
> Thanks for finding.
>
>
> >Barring objections, I'll push the attached patch to fix that.
>
> Please hold off - your colleagues found this before, and I worked on getting
> test coverage for the code. It's scheduled for commit together today.
> Unfortunately nobody looked at the test much...
\
And pushed, as
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=aa4b8c61d2cd57b53be03defb04d59b232a0e150
with the part that wasn't covered by tests now covered by
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=08e2edc0767ab6e619970f165cb34d4673105f23
Greetings,
Andres Freund
