Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

2020-12-12 Thread Peter Eisentraut 

On 2020-12-11 21:27, Alvaro Herrera wrote:

By the way--  What did you think of the idea of explictly marking the
types used for bitmasks using types bits32 and friends, instead of plain
int, which is harder to spot?


If we want to make it clearer, why not turn the thing into a struct, as 
in the attached patch, and avoid the bit fiddling altogether.
From d3125b3bfb8a3dc23e38f38bcf850ca6fc36f492 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 12 Dec 2020 09:17:55 +0100
Subject: [PATCH] Convert reindex options to struct

---
 src/backend/catalog/index.c  | 19 ---
 src/backend/commands/cluster.c   |  2 +-
 src/backend/commands/indexcmds.c | 94 
 src/backend/commands/tablecmds.c |  2 +-
 src/backend/tcop/utility.c   |  4 +-
 src/include/catalog/index.h  | 16 +++---
 src/include/commands/defrem.h|  9 +--
 7 files changed, 74 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..06342fddf1 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
  */
 void
 reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
- int options)
+ ReindexOptions options)
 {
RelationiRel,
heapRelation;
@@ -3602,7 +3602,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, 
char persistence,
IndexInfo  *indexInfo;
volatile bool skipped_constraint = false;
PGRUsageru0;
-   boolprogress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
+   boolprogress = options.REINDEXOPT_REPORT_PROGRESS;
 
pg_rusage_init(&ru0);
 
@@ -3611,12 +3611,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, 
char persistence,
 * we only need to be sure no schema or data changes are going on.
 */
heapId = IndexGetRelation(indexId,
- (options & 
REINDEXOPT_MISSING_OK) != 0);
+ 
options.REINDEXOPT_MISSING_OK);
/* if relation is missing, leave */
if (!OidIsValid(heapId))
return;
 
-   if ((options & REINDEXOPT_MISSING_OK) != 0)
+   if (options.REINDEXOPT_MISSING_OK)
heapRelation = try_table_open(heapId, ShareLock);
else
heapRelation = table_open(heapId, ShareLock);
@@ -3792,7 +3792,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, 
char persistence,
}
 
/* Log what we did */
-   if (options & REINDEXOPT_VERBOSE)
+   if (options.REINDEXOPT_VERBOSE)
ereport(INFO,
(errmsg("index \"%s\" was reindexed",
get_rel_name(indexId)),
@@ -3846,7 +3846,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, 
char persistence,
  * index rebuild.
  */
 bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions options)
 {
Relationrel;
Oid toast_relid;
@@ -3861,7 +3861,7 @@ reindex_relation(Oid relid, int flags, int options)
 * to prevent schema and data changes in it.  The lock level used here
 * should match ReindexTable().
 */
-   if ((options & REINDEXOPT_MISSING_OK) != 0)
+   if (options.REINDEXOPT_MISSING_OK)
rel = try_table_open(relid, ShareLock);
else
rel = table_open(relid, ShareLock);
@@ -3965,8 +3965,9 @@ reindex_relation(Oid relid, int flags, int options)
 * Note that this should fail if the toast relation is missing, 
so
 * reset REINDEXOPT_MISSING_OK.
 */
-   result |= reindex_relation(toast_relid, flags,
-  options & 
~(REINDEXOPT_MISSING_OK));
+   ReindexOptions newoptions = options;
+   newoptions.REINDEXOPT_MISSING_OK = false;
+   result |= reindex_relation(toast_relid, flags, newoptions);
}
 
return result;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fd5a6eec86..b0aa3536d1 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1412,7 +1412,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
 
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
 
-   reindex_relation(OIDOldHeap, reindex_flags, 0);
+   reindex_relation(OIDOldHeap, reindex_flags, (ReindexOptions){});
 
/* Report that we are now doing clean up */
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE

Suggestion: optionally return default value instead of error on failed cast

2020-12-12 Thread Wolfgang Walther 

Hi,

currently a failed cast throws an error. It would be useful to have a 
way to get a default value instead.


T-SQL has try_cast [1]
Oracle has CAST(... AS .. DEFAULT ... ON CONVERSION ERROR) [2]

The DEFAULT ... ON CONVERSION ERROR syntax seems like it could be 
implemented in PostgreSQL. Even if only DEFAULT NULL was supported (at 
first) that would already help.


The short syntax could be extended for the DEFAULT NULL case, too:

SELECT '...'::type -- throws error
SELECT '...':::type -- returns NULL

I couldn't find any previous discussion on this, please advise in case I 
just missed it.


Thoughts?

Best

Wolfgang

[1]: 
https://docs.microsoft.com/en-us/sql/t-sql/functions/try-cast-transact-sql
[2]: 
https://docs.oracle.com/en/database/oracle/oracle-database/12.2/sqlrf/CAST.html

Re: Asynchronous Append on postgres_fdw nodes.

2020-12-12 Thread Etsuro Fujita 
On Fri, Nov 20, 2020 at 3:51 PM Kyotaro Horiguchi
 wrote:
> At Tue, 17 Nov 2020 18:56:02 +0900, Etsuro Fujita  
> wrote in
> > * In Robert's patch [1] (and Horiguchi-san's, which was created based
> > on Robert's), ExecAppend() was modified to retrieve tuples from
> > async-aware children *before* the tuples will be needed, but I don't
>
> The "retrieve" means the move of a tuple from fdw to executor
> (ExecAppend or ExecAsync) layer?

Yes, that's what I mean.

> > think that's really a good idea, because the query might complete
> > before returning the tuples.  So I modified that function so that a
>
> I'm not sure how it matters. Anyway the fdw holds up to tens of tuples
> before the executor actually make requests for them.  The reason for
> the early fetching is letting fdw send the next request as early as
> possible. (However, I didn't measure the effect of the
> nodeAppend-level prefetching.)

I agree that that would lead to an improved efficiency in some cases,
but I still think that that would be useless in some other cases like
SELECT * FROM sharded_table LIMIT 1.  Also, I think the situation
would get worse if we support Append on top of joins or aggregates
over ForeignScans, which would be more expensive to perform than these
ForeignScans.

If we do prefetching, I think it would be better that it’s the
responsibility of the FDW to do prefetching, and I think that that
could be done by letting the FDW to start another data fetch,
independently of the core, in the ForeignAsyncNotify callback routine,
which I revived from Robert's original patch.  I think that that would
be more efficient, because the FDW would no longer need to wait until
all buffered tuples are returned to the core.  In the WIP patch, I
only allowed the callback routine to put the corresponding ForeignScan
node into a state where it’s either ready for a new request or needing
a callback for another data fetch, but I think we could probably relax
the restriction so that the ForeignScan node can be put into another
state where it’s ready for a new request while needing a callback for
the prefetch.

> > tuple is retrieved from an async-aware child *when* it is needed, like
> > Thomas' patch.  I used FDW callback functions proposed by Robert, but
> > introduced another FDW callback function ForeignAsyncBegin() for each
> > async-aware child to start an asynchronous data fetch at the first
> > call to ExecAppend() after ExecInitAppend() or ExecReScanAppend().
>
> Even though the terminology is not officially determined, in the past
> discussions "async-aware" meant "can handle async-capable subnodes"
> and "async-capable" is used as "can run asynchronously".

Thanks for the explanation!

> Likewise you
> seem to have changed the meaning of as_needrequest from "subnodes that
> needs to request for the next tuple" to "subnodes that already have
> got query-send request and waiting for the result to come".

No.  I think I might slightly change the original definition of
as_needrequest, though.

> I would
> argue to use the words and variables (names) in such meanings.

I think the word "aware" has a broader meaning, so the naming as
proposed would be OK IMO.  But actually, I don't have any strong
opinion about that, so I'll change it as explained.

> > * For EvalPlanQual, I modified the patch so that async-aware children
> > are treated as if they were synchronous when executing EvalPlanQual.
>
> Doesn't async execution accelerate the epq-fetching?  Or does
> async-execution goes into trouble in the EPQ path?

The reason why I disabled async execution when executing EPQ is to
avoid sending asynchronous queries to the remote sides, which would be
useless, because scan tuples for an EPQ recheck are obtained in a
dedicated way.

> > * In Robert's patch, all async-aware children below Append nodes in
> > the query waiting for events to occur were managed by a single EState,
> > but I modified the patch so that such children are managed by each
> > Append node, like Horiguchi-san's patch and Thomas'.
>
> Managing in Estate give advantage for push-up style executor but
> managing in node_state is simpler.

What do you mean by "push-up style executor"?

Thanks for the review!  Sorry for the delay.

Best regards,
Etsuro Fujita

Re: Asynchronous Append on postgres_fdw nodes.

2020-12-12 Thread Etsuro Fujita 
On Fri, Nov 20, 2020 at 8:16 PM Kyotaro Horiguchi
 wrote:
> I looked through the nodeAppend.c and postgres_fdw.c part and those
> are I think the core of this patch.

Thanks again for the review!

> -* figure out which subplan we are currently processing
> +* try to get a tuple from async subplans
> +*/
> +   if (!bms_is_empty(node->as_needrequest) ||
> +   (node->as_syncdone && 
> !bms_is_empty(node->as_asyncpending)))
> +   {
> +   if (ExecAppendAsyncGetNext(node, &result))
> +   return result;
>
> The function ExecAppendAsyncGetNext() is a function called only here,
> and contains only 31 lines.  It doesn't seem to me that the separation
> makes the code more readable.

Considering the original ExecAppend() is about 50 lines long, 31 lines
of code would not be small.  So I'd vote for separating it into
another function as proposed.

> -   /* choose new subplan; if none, we're done */
> -   if (!node->choose_next_subplan(node))
> +   /* wait or poll async events */
> +   if (!bms_is_empty(node->as_asyncpending))
> +   {
> +   Assert(!node->as_syncdone);
> +   Assert(bms_is_empty(node->as_needrequest));
> +   ExecAppendAsyncEventWait(node);
>
> You moved the function to wait for events from execAsync to
> nodeAppend. The former is a generic module that can be used from any
> kind of executor nodes, but the latter is specialized for nodeAppend.
> In other words, the abstraction level is lowered here.  What is the
> reason for the change?

The reason is just because that function is only called from
ExecAppend().  I put some functions only called from nodeAppend.c in
execAsync.c, though.

> +   /* Perform the actual callback. */
> +   ExecAsyncRequest(areq);
> +   if (ExecAppendAsyncResponse(areq))
> +   {
> +   Assert(!TupIsNull(areq->result));
> +   *result = areq->result;
>
> Putting aside the name of the functions, the first two function are
> used only this way at only two places. ExecAsyncRequest(areq) tells
> fdw to store the first tuple among the already received ones to areq,
> and ExecAppendAsyncResponse(areq) is checking the result is actually
> set. Finally the result is retrieved directory from areq->result.
> What is the reason that the two functions are separately exists?

I think that when an async-aware node gets a tuple from an
async-capable node, they should use ExecAsyncRequest() /
ExecAyncHogeResponse() rather than ExecProcNode() [1].  I modified the
patch so that ExecAppendAsyncResponse() is called from Append, but to
support bubbling up the plan tree discussed in [2], I think it should
be called from ForeignScans (the sides of async-capable nodes).  Am I
right?  Anyway, I’ll rename ExecAppendAyncResponse() to the one
proposed in Robert’s original patch.

> +   /* Perform the actual callback. */
> +   ExecAsyncNotify(areq);
>
> Mmm. The usage of the function (or its name) looks completely reverse
> to me.  I think FDW should NOTIFY to exec nodes that the new tuple
> gets available but the reverse is nonsense. What the function is
> actually doing is to REQUEST fdw to fetch tuples that are expected to
> have arrived, which is different from what the name suggests.

As mentioned in a previous email, this is an FDW callback routine
revived from Robert’s patch.  I think the naming is reasonable,
because the callback routine notifies the FDW of readiness of a file
descriptor.  And actually, the callback routine tells the core whether
the corresponding ForeignScan node is ready for a new request or not,
by setting the callback_pending flag accordingly.

> postgres_fdw.c
>
> > postgresIterateForeignScan(ForeignScanState *node)
> > {
> >   PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
> >   TupleTableSlot *slot = node->ss.ss_ScanTupleSlot;
> >
> >   /*
> >* If this is the first call after Begin or ReScan, we need to create 
> > the
> >* cursor on the remote side.
> >*/
> >   if (!fsstate->cursor_exists)
> >   create_cursor(node);
>
> With the patch, cursors are also created in another place so at least
> the comment is wrong.

Good catch!  Will fix.

> That being said, I think we should unify the
> code except the differences between async and sync.  For example, if
> the fetch_more_data_begin() needs to be called only for async
> fetching, the cursor should be created before calling the function, in
> the code path common with sync fetching.

I think that that would make the code easier to understand, but I’m
not 100% sure we really need to do so.

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/CA%2BTgmoYrbgTBnLwnr1v%3Dp

Re: Feature Proposal: Add ssltermination parameter for SNI-based LoadBalancing

2020-12-12 Thread Lukas Meisegeier 

Thanks for the provided ideas :)
I use HaProxy for my load-balancing and unfortunately I can't define
that I want to listen on a port for both ssl and non ssl requests.
That means if I try to return a fixed response 'S' on the SSLRequest it
fails with an SSL-Handshake failure cause the server expects a ssl message.

I searched for some way to forward to a default backend on ssl failure
but this seems to be no use-case for haproxy and isn't supported.

I also didn't find any other tcp-loadbalancer, which could handle this
type of ssl-failure fallback.

My only option would therefore be to write a custom loadbalancer for
this usecase, which is not really feasible given the amount of features
of haproxy I plan to use.

I have to say the psql ssl handshake procedure is really unique and
challenging :D

The tool stunnel is capable of this protocol but I can't do sni-based
loadbalancing with it so this is kind of a dead end here.

Lukas

Am 11-Dec-20 um 16:44 schrieb Heikki Linnakangas:

On 11/12/2020 16:46, Lukas Meisegeier wrote:

Hey Heikki,

thanks for providing feedback :)
The traffic between proxy and psql-server is unencrypted thats why I
don't need to patch the server.


Ok.


I tried returning a fixed response on the first plain SSLRequest
forwarding it to a psql-server with ssl enabled an tried to switch then
on the ssl connection startup but that didn't work out. I guess its
because the psql-server won't accept an ssl connection if its not
requested via SSLRequest.


Your proxy could receive the client's SSLRequest message, and respond
with a single byte 'S'. You don't need to forward that to the real
PostgreSQL server, since the connection to the PostgreSQL server is
unencrypted. Then perform the TLS handshake, and forward all traffic to
the real server only after that.

Client: -> SSLRequest
  Proxy: <- 'S'
Client: -> TLS ClientHello
  Proxy: [finish TLS handshake]

- Heikki

Re: Insert Documentation - Returning Clause and Order

2020-12-12 Thread James Coleman 
On Friday, December 11, 2020, David G. Johnston
 wrote:
>
> On Fri, Dec 11, 2020 at 6:24 AM Ashutosh Bapat  
> wrote:
>>
>> On Thu, Dec 10, 2020 at 7:49 PM David G. Johnston
>>  wrote:
>>
>> > Yeah, the ongoing work on parallel inserts would seem to be an issue.  We 
>> > should probably document that though.  And maybe as part of parallel 
>> > inserts patch provide a user-specifiable way to ask for such a guarantee 
>> > if needed.  ‘Insert returning ordered”
>>
>> I am curious about the usecase which needs that guarantee? Don't you
>> have a column on which you can ORDER BY so that it returns the same
>> order as INSERT?
>
>
> This comes up periodically in the context of auto-generated keys being 
> returned - specifically on the JDBC project list (maybe elsewhere...).  If 
> one adds 15 VALUES entries to an insert and then sends them in bulk to the 
> server it would be helpful if the generated keys could be matched up 
> one-to-one with the keyless objects in the client.  Basically "pipelining" 
> the client and server.

That’s a great use case. It’s not so much about ordering, per se, but
about identity.

Certainly almost every ORM, and maybe even other forms of application
code, need to be able to associate the serial column value returned
with what it inserted. I'd expect something like that (whether by
ordering explicitly or by providing some kind of mapping between
indexes in the statement data and the inserted/returned row values).

James

Re: Insert Documentation - Returning Clause and Order

2020-12-12 Thread David G. Johnston 
On Sat, Dec 12, 2020 at 7:02 AM James Coleman  wrote:

>
> Certainly almost every ORM, and maybe even other forms of application
> code, need to be able to associate the serial column value returned
> with what it inserted.
>

Yet most ORM would perform single inserts at a time, not in bulk, making
such a feature irrelevant to them.

I don't think having such a feature is all that important personally, but
the question comes every so often and it would be nice to be able to point
at the documentation for a definitive answer - not just one inferred from a
lack of documentation - especially since the observed behavior is that
order is preserved today.

David J.

Re: pglz compression performance, take two

2020-12-12 Thread Andrey Borodin 



> 9 дек. 2020 г., в 12:44, Andrey Borodin  написал(а):
> PFA the patch with some editorialisation by me.
> I saw some reports of bottlenecking in pglz WAL compression [1].

I've checked that on my machine simple test
echo "wal_compression = on" >> $PGDATA/postgresql.conf
pgbench -i -s 20 && pgbench -T 30
shows ~2-3% of improvement, but the result is not very stable, deviation is 
comparable. In fact, bottleneck is just shifted from pglz, thus impact is not 
that measurable.

I've found out that the patch continues ideas from thread [0] and commit 
031cc55 [1], but in much more shotgun-surgery way.
Out of curiosity I've rerun tests from that thread
postgres=# with patched as (select testname, avg(seconds) patched from 
testresults0 group by testname),unpatched as (select testname, avg(seconds) 
unpatched from testresults group by testname) select * from unpatched join 
patched using (testname);
 testname  |   unpatched|patched 
---++
 512b random   | 4.55680150 | 4.35129800
 100k random   | 1.03342300 | 1.00326200
 100k of same byte | 2.16897150 | 2.09581550
 2k random | 3.16138150 | 3.18613500
 512b text | 5.72336000 | 5.36023300
 5k text   | 1.70448350 | 1.80867700
(6 rows)


Results of direct call are somewhat more clear.
Unpatched:
 testname  |   auto
---+---
 5k text   |  1100.705
 512b text |   240.585
 2k random |   106.865
 100k random   | 2.663
 512b random   |   145.736
 100k of same byte | 13426.880
(6 rows)

Patched:
 testname  |   auto   
---+--
 5k text   |  767.535
 512b text |  159.076
 2k random |   77.126
 100k random   |1.698
 512b random   |   95.768
 100k of same byte | 6035.159
(6 rows)

Thanks!

Best regards, Andrey Borodin.


[0] https://www.postgresql.org/message-id/flat/5130C914.8080106%40vmware.com
[1] 
https://github.com/x4m/postgres_g/commit/031cc55bbea6b3a6b67c700498a78fb1d4399476

Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

2020-12-12 Thread Justin Pryzby 
On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> On 2020-12-11 21:27, Alvaro Herrera wrote:
> > By the way--  What did you think of the idea of explictly marking the
> > types used for bitmasks using types bits32 and friends, instead of plain
> > int, which is harder to spot?
> 
> If we want to make it clearer, why not turn the thing into a struct, as in
> the attached patch, and avoid the bit fiddling altogether.

I like this.
It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
with an "int options" bitmask which is passed to reindex_index() et al.  Your
patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
which I think is good.

So I've rebased this branch on your patch.

Some thoughts:

 - what about removing the REINDEXOPT_* prefix ?
 - You created local vars with initialization like "={}". But I thought it's
   needed to include at least one struct member like "={false}", or else
   they're not guaranteed to be zerod ?
 - You passed the structure across function calls.  The usual convention is to
   pass a pointer.

I also changed the errcode and detail for this one.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("incompatible TABLESPACE option"),
errdetail("TABLESPACE can only be used with VACUUM FULL.")));

-- 
Justin
>From 3d84ec564a1ad3df50967153d2acbd443a04b864 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 12 Dec 2020 09:17:55 +0100
Subject: [PATCH v34 1/8] Convert reindex options to struct

---
 src/backend/catalog/index.c  | 19 ---
 src/backend/commands/cluster.c   |  2 +-
 src/backend/commands/indexcmds.c | 94 
 src/backend/commands/tablecmds.c |  2 +-
 src/backend/tcop/utility.c   |  4 +-
 src/include/catalog/index.h  | 16 +++---
 src/include/commands/defrem.h|  9 +--
 7 files changed, 74 insertions(+), 72 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 731610c701..06342fddf1 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3594,7 +3594,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
  */
 void
 reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
-			  int options)
+			  ReindexOptions options)
 {
 	Relation	iRel,
 heapRelation;
@@ -3602,7 +3602,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 	IndexInfo  *indexInfo;
 	volatile bool skipped_constraint = false;
 	PGRUsage	ru0;
-	bool		progress = (options & REINDEXOPT_REPORT_PROGRESS) != 0;
+	bool		progress = options.REINDEXOPT_REPORT_PROGRESS;
 
 	pg_rusage_init(&ru0);
 
@@ -3611,12 +3611,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 	 * we only need to be sure no schema or data changes are going on.
 	 */
 	heapId = IndexGetRelation(indexId,
-			  (options & REINDEXOPT_MISSING_OK) != 0);
+			  options.REINDEXOPT_MISSING_OK);
 	/* if relation is missing, leave */
 	if (!OidIsValid(heapId))
 		return;
 
-	if ((options & REINDEXOPT_MISSING_OK) != 0)
+	if (options.REINDEXOPT_MISSING_OK)
 		heapRelation = try_table_open(heapId, ShareLock);
 	else
 		heapRelation = table_open(heapId, ShareLock);
@@ -3792,7 +3792,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 	}
 
 	/* Log what we did */
-	if (options & REINDEXOPT_VERBOSE)
+	if (options.REINDEXOPT_VERBOSE)
 		ereport(INFO,
 (errmsg("index \"%s\" was reindexed",
 		get_rel_name(indexId)),
@@ -3846,7 +3846,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
  * index rebuild.
  */
 bool
-reindex_relation(Oid relid, int flags, int options)
+reindex_relation(Oid relid, int flags, ReindexOptions options)
 {
 	Relation	rel;
 	Oid			toast_relid;
@@ -3861,7 +3861,7 @@ reindex_relation(Oid relid, int flags, int options)
 	 * to prevent schema and data changes in it.  The lock level used here
 	 * should match ReindexTable().
 	 */
-	if ((options & REINDEXOPT_MISSING_OK) != 0)
+	if (options.REINDEXOPT_MISSING_OK)
 		rel = try_table_open(relid, ShareLock);
 	else
 		rel = table_open(relid, ShareLock);
@@ -3965,8 +3965,9 @@ reindex_relation(Oid relid, int flags, int options)
 		 * Note that this should fail if the toast relation is missing, so
 		 * reset REINDEXOPT_MISSING_OK.
 		 */
-		result |= reindex_relation(toast_relid, flags,
-   options & ~(REINDEXOPT_MISSING_OK));
+		ReindexOptions newoptions = options;
+		newoptions.REINDEXOPT_MISSING_OK = false;
+		result |= reindex_relation(toast_relid, flags, newoptions);
 	}
 
 	return result;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fd5a6eec86..b0aa3536d1 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1412,7 +1412,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 	pgsta

Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

2020-12-12 Thread Justin Pryzby 
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> > On 2020-12-11 21:27, Alvaro Herrera wrote:
> > > By the way--  What did you think of the idea of explictly marking the
> > > types used for bitmasks using types bits32 and friends, instead of plain
> > > int, which is harder to spot?
> > 
> > If we want to make it clearer, why not turn the thing into a struct, as in
> > the attached patch, and avoid the bit fiddling altogether.
> 
> I like this.
> It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
> In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
> with an "int options" bitmask which is passed to reindex_index() et al.  Your
> patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
> which I think is good.
> 
> So I've rebased this branch on your patch.

Also, the cfbot's windows VS compilation failed due to "compound literal",
which I don't think is used anywhere else.

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.120108

  src/backend/commands/cluster.c(1580): warning C4133: 'return' : incompatible 
types - from 'List *' to 'int *' [C:\projects\postgresql\postgres.vcxproj]
"C:\projects\postgresql\pgsql.sln" (default target) (1) ->
"C:\projects\postgresql\cyrillic_and_mic.vcxproj" (default target) (5) ->
"C:\projects\postgresql\postgres.vcxproj" (default target) (6) ->
(ClCompile target) ->
  src/backend/commands/cluster.c(1415): error C2059: syntax error : '}' 
[C:\projects\postgresql\postgres.vcxproj]
  src/backend/commands/cluster.c(1534): error C2143: syntax error : missing '{' 
before '*' [C:\projects\postgresql\postgres.vcxproj]
  src/backend/commands/cluster.c(1536): error C2371: 'get_tables_to_cluster' : 
redefinition; different basic types [C:\projects\postgresql\postgres.vcxproj]
  src/backend/commands/indexcmds.c(2462): error C2059: syntax error : '}' 
[C:\projects\postgresql\postgres.vcxproj]
  src/backend/commands/tablecmds.c(1894): error C2059: syntax error : '}' 
[C:\projects\postgresql\postgres.vcxproj]

My fix! patch resolves that.

-- 
Justin

Re: Insert Documentation - Returning Clause and Order

2020-12-12 Thread James Coleman 
On Sat, Dec 12, 2020 at 10:11 AM David G. Johnston
 wrote:
>
> On Sat, Dec 12, 2020 at 7:02 AM James Coleman  wrote:
>>
>>
>> Certainly almost every ORM, and maybe even other forms of application
>> code, need to be able to associate the serial column value returned
>> with what it inserted.
>
>
> Yet most ORM would perform single inserts at a time, not in bulk, making such 
> a feature irrelevant to them.

I think that's a pretty hasty generalization. It's the majority of use
cases in an ORM, sure, but plenty of ORMs (and libraries or
applications using them) support inserting batches where performance
requires it. Rails/ActiveRecord is actually integrating that feature
into core (though many Ruby libraries already add that support, as
does, for example, the application I spend the majority of time
working on).

James