On Sun, Feb 14, 2021 at 08:10:50PM -0600, Justin Pryzby wrote:
> Isn't this dead code ?
Nope, it's not dead. Those two code paths can be hit when attempting
a reidex with a tablespace move directly on toast tables and indexes,
see:
=# create table aa (a text);
CREATE TABLE
=# select relname from
On Thu, Feb 04, 2021 at 03:38:39PM +0900, Michael Paquier wrote:
> On Wed, Feb 03, 2021 at 07:54:42PM +0900, Michael Paquier wrote:
> > Not sure I like that. Here is a proposal:
> > "it is recommended to separately use ALTER TABLE ONLY on them so as
> > any new partitions attached inherit the new
On Wed, Feb 03, 2021 at 07:54:42PM +0900, Michael Paquier wrote:
> Not sure I like that. Here is a proposal:
> "it is recommended to separately use ALTER TABLE ONLY on them so as
> any new partitions attached inherit the new tablespace value."
So, I have done more work on this stuff today, and
On Wed, Feb 03, 2021 at 01:35:26PM +0300, Alexey Kondratov wrote:
> This check for OidIsValid() seems to be excessive, since you moved the whole
> ACL check under 'if (tablespacename != NULL)' here.
That's more consistent with ATPrepSetTableSpace().
> +SELECT relid, parentrelid, level FROM
>
On Wed, Feb 03, 2021 at 12:53:42AM -0600, Justin Pryzby wrote:
> On Wed, Feb 03, 2021 at 03:37:39PM +0900, Michael Paquier wrote:
>> index 627b36300c..4ee3951ca0 100644
>> --- a/doc/src/sgml/ref/reindex.sgml
>> +++ b/doc/src/sgml/ref/reindex.sgml
>> @@ -293,8 +311,30 @@ REINDEX [ ( >
On 2021-02-03 09:37, Michael Paquier wrote:
On Tue, Feb 02, 2021 at 10:32:19AM +0900, Michael Paquier wrote:
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
> index_create() with a proper
On Wed, Feb 03, 2021 at 03:37:39PM +0900, Michael Paquier wrote:
> index 627b36300c..4ee3951ca0 100644
> --- a/doc/src/sgml/ref/reindex.sgml
> +++ b/doc/src/sgml/ref/reindex.sgml
> @@ -293,8 +311,30 @@ REINDEX [ ( class="parameter">option [, ...] ) ] { IN
> respectively. Each partition of the
On Tue, Feb 02, 2021 at 10:32:19AM +0900, Michael Paquier wrote:
> On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> > Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
> > index_create() with a proper tablespaceOid instead of
> >
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
> index_create() with a proper tablespaceOid instead of
> SetRelationTableSpace(). And its checks structure is more restrictive even
> without tablespace
On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
> On 2021-01-30 05:23, Michael Paquier wrote:
> > This makes me really wonder if we would not be better to restrict this
> > operation for partitioned relation as part of REINDEX as a first step.
> > Another thing, mentioned
On 2021-01-30 05:23, Michael Paquier wrote:
On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote:
On 2021-01-28 14:42, Alexey Kondratov wrote:
No, you are right, we are doing REINDEX with AccessExclusiveLock as
it
was before. This part is a more specific one. It only applies to
On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote:
> On 2021-01-28 14:42, Alexey Kondratov wrote:
>> No, you are right, we are doing REINDEX with AccessExclusiveLock as it
>> was before. This part is a more specific one. It only applies to
>> partitioned indexes, which do not hold
On 2021-01-28 14:42, Alexey Kondratov wrote:
On 2021-01-28 00:36, Alvaro Herrera wrote:
I didn't look at the patch closely enough to understand why you're
trying to do something like CLUSTER, VACUUM FULL or REINDEX without
holding full AccessExclusiveLock on the relation. But do keep in mind
On 2021-01-28 00:36, Alvaro Herrera wrote:
On 2021-Jan-28, Alexey Kondratov wrote:
I have read more about lock levels and ShareLock should prevent any
kind of
physical modification of indexes. We already hold ShareLock doing
find_all_inheritors(), which is higher than
On 2021-Jan-28, Alexey Kondratov wrote:
> I have read more about lock levels and ShareLock should prevent any kind of
> physical modification of indexes. We already hold ShareLock doing
> find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so
> using ShareLock seems to be safe
Thanks for updating the patch. I have just a couple comments on the new (and
old) language.
On Thu, Jan 28, 2021 at 12:19:06AM +0300, Alexey Kondratov wrote:
> Also added tests for ACL checks, relfilenode changes. Added ACL recheck for
> multi-transactional case. Added info about TOAST index
On 2021-01-27 06:14, Michael Paquier wrote:
On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
In the new 0002 I moved ACL check to the upper level, i.e.
ExecReindex(),
and removed expensive text generation in test. Not touched yet some of
your
previously raised concerns. Also,
On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
> CheckRelationTableSpaceMove() does not feel like a right place for invoking
> post alter hooks. It is intended only to check for tablespace change
> possibility. Anyway, ATExecSetTableSpace() and
> ATExecSetTableSpaceNoStorage()
On 2021-01-26 09:58, Michael Paquier wrote:
On Mon, Jan 25, 2021 at 11:11:38PM +0300, Alexey Kondratov wrote:
I updated comment with CCI info, did pgindent run and renamed new
function
to SetRelationTableSpace(). New patch is attached.
[...]
Yeah, all these checks we complicated from the
On Mon, Jan 25, 2021 at 11:11:38PM +0300, Alexey Kondratov wrote:
> I updated comment with CCI info, did pgindent run and renamed new function
> to SetRelationTableSpace(). New patch is attached.
>
> [...]
>
> Yeah, all these checks we complicated from the beginning. I will try to find
> a better
On 2021-01-25 11:07, Michael Paquier wrote:
On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
I have updated patches accordingly and also simplified tablespaceOid
checks
and assignment in the newly added SetRelTableSpace(). Result is
attached as
two separate patches for an ease
On Mon, Jan 25, 2021 at 05:07:29PM +0900, Michael Paquier wrote:
> On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
> > I have updated patches accordingly and also simplified tablespaceOid checks
> > and assignment in the newly added SetRelTableSpace(). Result is attached as
> >
On 2021-01-22 00:26, Justin Pryzby wrote:
On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
Attached is a new patch set of first two patches, that should resolve
all
the issues raised before (ACL, docs, tests) excepting TOAST. Double
thanks
for suggestion to add more tests with
On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
> Attached is a new patch set of first two patches, that should resolve all
> the issues raised before (ACL, docs, tests) excepting TOAST. Double thanks
> for suggestion to add more tests with nested partitioning. I have found and
>
On 2021-01-21 17:06, Alexey Kondratov wrote:
On 2021-01-21 04:41, Michael Paquier wrote:
There are no tests for partitioned tables, aka we'd want to make sure
that the new partitioned index is on the correct tablespace, as well
as all its leaves. It may be better to have at least two levels
On 2021-01-21 04:41, Michael Paquier wrote:
On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
On 2021-Jan-20, Alexey Kondratov wrote:
Ugh, forgot to attach the patches. Here they are.
Yeah, looks reasonable.
+
+ if (changed)
+ /* Record dependency on
On Wed, Jan 20, 2021 at 03:34:39PM -0300, Alvaro Herrera wrote:
> On 2021-Jan-20, Alexey Kondratov wrote:
>> Ugh, forgot to attach the patches. Here they are.
>
> Yeah, looks reasonable.
Patch 0002 still has a whole set of issues as I pointed out a couple
of hours ago, but if we agree on 0001 as
On 2021-Jan-20, Alexey Kondratov wrote:
> On 2021-01-20 21:08, Alexey Kondratov wrote:
> >
> > I did a refactoring of ATExecSetTableSpaceNoStorage() in the 0001. New
> > function SetRelTablesapce() is placed into the tablecmds.c. Following
> > 0002 gets use of it. Is it close to what you and
On 2021-01-20 21:08, Alexey Kondratov wrote:
On 2021-01-20 18:54, Alvaro Herrera wrote:
On 2021-Jan-20, Alvaro Herrera wrote:
On 2021-Jan-20, Michael Paquier wrote:
> +/*
> + * This is mostly duplicating ATExecSetTableSpaceNoStorage,
> + * which should maybe be factored out to a library
On 2021-01-20 18:54, Alvaro Herrera wrote:
On 2021-Jan-20, Alvaro Herrera wrote:
On 2021-Jan-20, Michael Paquier wrote:
> +/*
> + * This is mostly duplicating ATExecSetTableSpaceNoStorage,
> + * which should maybe be factored out to a library function.
> + */
> Wouldn't it be better to do
On 2021-Jan-20, Alvaro Herrera wrote:
> On 2021-Jan-20, Michael Paquier wrote:
>
> > +/*
> > + * This is mostly duplicating ATExecSetTableSpaceNoStorage,
> > + * which should maybe be factored out to a library function.
> > + */
> > Wouldn't it be better to do first the refactoring of 0002 and
On 2021-Jan-20, Michael Paquier wrote:
> +/*
> + * This is mostly duplicating ATExecSetTableSpaceNoStorage,
> + * which should maybe be factored out to a library function.
> + */
> Wouldn't it be better to do first the refactoring of 0002 and then
> 0001 so as REINDEX can use the new routine,
On Mon, Jan 18, 2021 at 02:37:57AM -0600, Justin Pryzby wrote:
> Attached. I will re-review these myself tomorrow.
I have begun looking at 0001 and 0002...
+/*
+ * This is mostly duplicating ATExecSetTableSpaceNoStorage,
+ * which should maybe be factored out to a library function.
+ */
Hi,
For 0001-Allow-REINDEX-to-change-tablespace.patch :
+ * InvalidOid, use the tablespace in-use instead.
'in-use' seems a bit redundant in the sentence.
How about : InvalidOid, use the tablespace of the index instead.
Cheers
On Mon, Jan 18, 2021 at 12:38 AM Justin Pryzby wrote:
> On Mon,
On Mon, Jan 18, 2021 at 02:18:44PM +0900, Michael Paquier wrote:
> On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> > 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
> >
On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> 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.
On Thu, Jan 14, 2021 at 02:18:45PM +0900, Michael Paquier wrote:
> Indeed. Let's first wait a couple of days and see if others have any
> comments or objections about this v6.
Hearing nothing, I have looked at that again this morning and applied
v6 after a reindent and some adjustments in the
On Wed, Jan 13, 2021 at 04:39:40PM +0300, Alexey Kondratov wrote:
> + bits32 options;/* bitmask of
> CLUSTEROPT_* */
>
> This should say '/* bitmask of CLUOPT_* */', I guess, since there are only
> CLUOPT's defined. Otherwise, everything looks as per
On 2021-01-13 14:34, Michael Paquier wrote:
On Wed, Jan 13, 2021 at 05:22:49PM +0900, Michael Paquier wrote:
Yeah, that makes sense. I'll send an updated patch based on that.
And here you go as per the attached. I don't think that there was
anything remaining on my radar. This version
On Wed, Dec 23, 2020 at 07:30:35PM +0300, Alexey Kondratov wrote:
> After eyeballing the patch I can add that we should alter this comment:
>
> int options;/* bitmask of VacuumOption */
>
> as you are going to replace VacuumOption with VACOPT_* defs. So this should
> say:
>
>
On Thu, Dec 24, 2020 at 10:50:34AM +0900, Michael Paquier wrote:
> FWIW, it still makes the most sense to me to keep the options that are
> extracted from the grammar or things that apply to all the
> sub-routines of REINDEX to be tracked in a single structure, so this
> should include only the
On Wed, Dec 23, 2020 at 07:07:54PM -0600, Justin Pryzby wrote:
> On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote:
>> On 2020-Dec-23, Justin Pryzby wrote:
>>> This was getting ugly:
>>>
>>> extern void reindex_index(Oid indexId, bool skip_constraint_checks,
>>>
On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote:
> On 2020-Dec-23, Justin Pryzby wrote:
>
> > This was getting ugly:
> >
> > extern void reindex_index(Oid indexId, bool skip_constraint_checks,
> > char relpersistence, int options, Oid
> > tablespaceOid)Z
On 2020-Dec-23, Justin Pryzby wrote:
> This was getting ugly:
>
> extern void reindex_index(Oid indexId, bool skip_constraint_checks,
> char relpersistence, int options, Oid
> tablespaceOid)Z
Is this what I suggested?
On Wed, Dec 23, 2020 at 07:22:05PM -0300, Alvaro Herrera wrote:
> Also: it seems a bit weird to me to put the flags inside the options
> struct. I would keep them separate -- so initially the options struct
> would only have the tablespace OID, on API cleanliness grounds:
I don't see why they'd
On 2020-Dec-23, Michael Paquier wrote:
> bool
> -reindex_relation(Oid relid, int flags, int options)
> +reindex_relation(Oid relid, int flags, ReindexOptions *options)
> {
> Relationrel;
> Oid toast_relid;
Wait a minute. reindex_relation has 'flags' and
On 2020-12-23 10:38, Michael Paquier wrote:
On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:
Now, I really think utility.c ought to pass in a pointer to a local
ReindexOptions variable to avoid all the memory context, which is
unnecessary
and prone to error.
Yeah, it sounds
On 2020-12-23 08:22, Justin Pryzby wrote:
On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote:
Justin:
For reindex_index() :
+ if (options->tablespaceOid == MyDatabaseTableSpace)
+ options->tablespaceOid = InvalidOid;
...
+ oldTablespaceOid = iRel->rd_rel->reltablespace;
+
On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:
> Now, I really think utility.c ought to pass in a pointer to a local
> ReindexOptions variable to avoid all the memory context, which is unnecessary
> and prone to error.
Yeah, it sounds right to me to just bite the bullet and do
On Tue, Dec 22, 2020 at 03:22:19PM -0800, Zhihong Yu wrote:
> Justin:
> For reindex_index() :
>
> + if (options->tablespaceOid == MyDatabaseTableSpace)
> + options->tablespaceOid = InvalidOid;
> ...
> + oldTablespaceOid = iRel->rd_rel->reltablespace;
> + if (set_tablespace &&
> +
Justin:
For reindex_index() :
+ if (options->tablespaceOid == MyDatabaseTableSpace)
+ options->tablespaceOid = InvalidOid;
...
+ if (set_tablespace &&
+ (options->tablespaceOid != oldTablespaceOid ||
+ (options->tablespaceOid == MyDatabaseTableSpace &&
On Tue, Dec 22, 2020 at 06:57:41PM +0900, Michael Paquier wrote:
> On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:
> > Also, this one is going to be subsumed by ExecReindex(), so the palloc will
> > go
> > away (otherwise I would ask to pass it in from the caller):
>
> Yeah,
On Tue, Dec 22, 2020 at 02:32:05AM -0600, Justin Pryzby wrote:
> Also, this one is going to be subsumed by ExecReindex(), so the palloc will go
> away (otherwise I would ask to pass it in from the caller):
Yeah, maybe. Still you need to be very careful if you have any
allocated variables like a
On Tue, Dec 22, 2020 at 03:47:57PM +0900, Michael Paquier wrote:
> On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:
> > On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:
> > > I don't like this idea too much, because adding an option causes an ABI
> > > break. I
On Wed, Dec 16, 2020 at 10:01:11AM +0900, Michael Paquier wrote:
> On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:
> > I don't like this idea too much, because adding an option causes an ABI
> > break. I don't think we commonly add options in backbranches, but it
> > has happened.
On Tue, Dec 15, 2020 at 09:45:17PM -0300, Alvaro Herrera wrote:
> I don't like this idea too much, because adding an option causes an ABI
> break. I don't think we commonly add options in backbranches, but it
> has happened. The bitmask is much easier to work with in that regard.
ABI
On 2020-Dec-12, 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
On Mon, Dec 14, 2020 at 06:14:17PM -0600, Justin Pryzby wrote:
> 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
On 2020-12-15 03:14, Justin Pryzby wrote:
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
> >
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
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
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
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
On Fri, Dec 11, 2020 at 05:27:03PM -0300, 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?
Right, we could just do that while the area is
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?
On 2020-12-05 02:30, Michael Paquier wrote:
On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote:
FWIW I'm with Peter on this.
Okay, attached is a patch to adjust the enums for the set of utility
commands that is the set of things I have touched lately. Should that
be extended
On Fri, Dec 04, 2020 at 04:28:26PM -0300, Alvaro Herrera wrote:
> FWIW I'm with Peter on this.
Okay, attached is a patch to adjust the enums for the set of utility
commands that is the set of things I have touched lately. Should that
be extended more? I have not done that as a lot of those
On Fri, Dec 04, 2020 at 09:40:31PM +0300, Alexey Kondratov wrote:
> > I liked the bools, but dropped them so the patch is smaller.
>
> I had a look on 0001 and it looks mostly fine to me except some strange
> mixture of tabs/spaces in the ExecReindex(). There is also a couple of
> meaningful
On 2020-Dec-04, Michael Paquier wrote:
> VacuumOption does that since 6776142, and ClusterOption since 9ebe057,
> so switching ReindexOption to just match the two others still looks
> like the most consistent move.
9ebe057 goes to show why this is a bad idea, since it has this:
+typedef enum
On 2020-12-04 04:25, Justin Pryzby wrote:
On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
> +typedef struct ReindexParams {
> + bool concurrently;
> + bool verbose;
> + bool missingok;
> +
> + int options;/* bitmask of lowlevel REINDEXOPT_* */
> +} ReindexParams;
> +
On Thu, Dec 03, 2020 at 08:46:09PM +0100, Peter Eisentraut wrote:
> There are a couple of more places like this, including the existing
> ClusterOption that this patched moved around, but we should be removing
> those.
>
> My reasoning is that if you look at an enum value of this type, either say
On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
> > +typedef struct ReindexParams {
> > + bool concurrently;
> > + bool verbose;
> > + bool missingok;
> > +
> > + int options;/* bitmask of lowlevel REINDEXOPT_* */
> > +} ReindexParams;
> > +
>
> By moving everything
A side comment on this patch: I think using enums as bit mask values is
bad style. So changing this:
-/* Reindex options */
-#define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */
-#define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */
-#define REINDEXOPT_MISSING_OK
On Wed, Dec 02, 2020 at 10:30:08PM -0600, Justin Pryzby wrote:
> Good idea. I think you mean like this.
Yes, something like that. Thanks.
> +typedef struct ReindexParams {
> + bool concurrently;
> + bool verbose;
> + bool missingok;
> +
> + int options;/* bitmask of
On Thu, Dec 03, 2020 at 10:19:43AM +0900, Michael Paquier wrote:
> OK, this one is now committed. As of this thread, I think that we are
> going to need to do a bit more once we add options that are not just
> booleans for both commands (the grammar rules do not need to be
> changed now):
> -
On Tue, Dec 01, 2020 at 03:10:13PM +0900, Michael Paquier wrote:
> Well, my impression is that both of you kept exchanging patches,
> touching and reviewing each other's patch (note that Alexei has also
> sent a rebase of 0002 just yesterday), so I think that it is fair to
> say that both of you
On Mon, Nov 30, 2020 at 11:43:08PM -0600, Justin Pryzby wrote:
> I eyeballed the patch and rebased the rest of the series on top if it to play
> with. Looks fine - thanks.
Cool, thanks.
> FYI, the commit messages have the proper "author" for attribution. I proposed
> and implemented the
On Tue, Dec 01, 2020 at 11:46:55AM +0900, Michael Paquier wrote:
> On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:
> > Thanks. I have rebased the remaining patches on top of 873ea9ee to use
> > 'utility_option_list' instead of 'common_option_list'.
>
> Thanks, that helps a lot.
On Mon, Nov 30, 2020 at 05:12:42PM +0300, Alexey Kondratov wrote:
> Thanks. I have rebased the remaining patches on top of 873ea9ee to use
> 'utility_option_list' instead of 'common_option_list'.
Thanks, that helps a lot. I have gone through 0002, and tweaked it as
the attached (note that this
On 2020-11-30 14:33, Michael Paquier wrote:
On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote:
@cfbot: rebased
Catching up with the activity here, I can see four different things in
the patch set attached:
1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX
to
On Tue, Nov 24, 2020 at 09:31:23AM -0600, Justin Pryzby wrote:
> @cfbot: rebased
Catching up with the activity here, I can see four different things in
the patch set attached:
1) Refactoring of the grammar of CLUSTER, VACUUM, ANALYZE and REINDEX
to support values in parameters.
2) Tablespace
On Sat, Oct 31, 2020 at 01:36:11PM -0500, Justin Pryzby wrote:
> > > From the grammar perspective ANY option is available for any command
> > > that uses parenthesized option list. All the checks and validations
> > > are performed at the corresponding command code.
> > > This analyze_keyword is
On Wed, Sep 23, 2020 at 07:43:01PM +0300, Alexey Kondratov wrote:
> On 2020-09-09 18:36, Justin Pryzby wrote:
> > Rebased on a6642b3ae: Add support for partitioned tables and indexes in
> > REINDEX
> >
> > So now this includes the new functionality and test for reindexing a
> > partitioned table
On Wed, Sep 09, 2020 at 09:22:00PM +0900, Michael Paquier wrote:
> On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
> > Initially I added List *params, and Michael suggested to retire
> > ReindexStmt->concurrent. I provided a patch to do so, initially by leaving
> > int
> > options
On 2020-09-09 15:22, Michael Paquier wrote:
On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
Initially I added List *params, and Michael suggested to retire
ReindexStmt->concurrent. I provided a patch to do so, initially by
leaving int
options and then, after this, removing it
On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
> Initially I added List *params, and Michael suggested to retire
> ReindexStmt->concurrent. I provided a patch to do so, initially by leaving
> int
> options and then, after this, removing it to "complete the thought", and get
> rid
On Tue, Sep 08, 2020 at 09:02:38PM -0300, Alvaro Herrera wrote:
> On 2020-Sep-08, Justin Pryzby wrote:
>
> > From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001
> > From: Justin Pryzby
> > Date: Fri, 27 Mar 2020 17:50:46 -0500
> > Subject: [PATCH v27 1/5] Change
On 2020-Sep-08, Justin Pryzby wrote:
> From 992e0121925c74d5c5a4e5b132cddb3d6b31da86 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby
> Date: Fri, 27 Mar 2020 17:50:46 -0500
> Subject: [PATCH v27 1/5] Change REINDEX/CLUSTER to accept an option list..
>
> ..like EXPLAIN (..), VACUUM (..), and
On Thu, Sep 03, 2020 at 09:43:51PM -0500, Justin Pryzby wrote:
> And rebased on Michael's commit removing ReindexStmt->concurrent.
Rebased on a6642b3ae: Add support for partitioned tables and indexes in REINDEX
So now this includes the new functionality and test for reindexing a
partitioned
On Wed, Sep 02, 2020 at 06:07:06PM -0500, Justin Pryzby wrote:
> On my side, I've also rearranged function parameters to make the diff more
> readable. And squishes your changes into the respective patches.
This resolves a breakage I failed to notice from a last-minute edit.
And squishes two
On Thu, Sep 03, 2020 at 12:00:17AM +0300, Alexey Kondratov wrote:
> On 2020-09-02 07:56, Justin Pryzby wrote:
> >
> > Done in the attached, which is also rebased on 1d6541666.
> >
> > And added RelationAssumeNewRelfilenode() as I mentioned - but I'm hoping
> > to
> > hear from Michael about any
On Tue, Sep 01, 2020 at 09:24:10PM -0500, Justin Pryzby wrote:
> On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote:
> > On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:
> > > On 2020-Sep-01, Justin Pryzby wrote:
> > >> The question isn't whether to use a parenthesized
On Wed, Sep 02, 2020 at 10:00:12AM +0900, Michael Paquier wrote:
> On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:
> > On 2020-Sep-01, Justin Pryzby wrote:
> >> The question isn't whether to use a parenthesized option list. I realized
> >> that
> >> long ago (even though Alexey
On Tue, Sep 01, 2020 at 09:29:28PM -0400, Alvaro Herrera wrote:
> Seems sensible, but only to be done when actually needed, right?
Of course.
--
Michael
signature.asc
Description: PGP signature
On 2020-Sep-02, Michael Paquier wrote:
> Yeah, I am all for removing "concurrent" from ReindexStmt, but I don't
> think that the proposed 0002 is that, because it is based on the
> assumption that we'd want more than just boolean-based options in
> those statements, and this case is not justified
On Tue, Sep 01, 2020 at 11:48:30AM -0400, Alvaro Herrera wrote:
> On 2020-Sep-01, Justin Pryzby wrote:
>> The question isn't whether to use a parenthesized option list. I realized
>> that
>> long ago (even though Alexey didn't initially like it). Check 0002, which
>> gets
>> rid of "bool
On 2020-Sep-01, Justin Pryzby wrote:
> On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote:
> > The advantage of using a parenthesized option list is that you can add
> > *further* options without making the new keywords reserved. Of course,
> > we already reserve CONCURRENTLY and
On Tue, Sep 01, 2020 at 11:40:18AM -0400, Alvaro Herrera wrote:
> On 2020-Aug-11, Justin Pryzby wrote:
> > On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote:
>
> > > The grammar that has been committed was the one that for the most
> > > support, so we need to live with that. I
On 2020-Aug-11, Justin Pryzby wrote:
> On Tue, Aug 11, 2020 at 02:39:45PM +0900, Michael Paquier wrote:
> > The grammar that has been committed was the one that for the most
> > support, so we need to live with that. I wonder if we should simplify
> > ReindexStmt and move the "concurrent" flag
On 2020-09-01 13:12, Justin Pryzby wrote:
This patch seems to be missing a call to RelationAssumeNewRelfilenode()
in
reindex_index().
That's maybe the related to the cause of the crashes I pointed out
earlier this
year.
Alexey's v4 patch changed RelationSetNewRelfilenode() to accept a
1 - 100 of 160 matches
Mail list logo
