On Tue, May 21, 2024 at 08:33:51AM -0500, Justin Pryzby wrote:
> It occurred to me that psql \dP+ should show the AM of partitioned
> tables (and other partitioned rels).
> Arguably, this could've been done when \dP was introduced in v12, but
> at that point would've shown the AM only for
On Tue, May 21, 2024 at 08:33:51AM -0500, Justin Pryzby wrote:
> It occurred to me that psql \dP+ should show the AM of partitioned
> tables (and other partitioned rels).
ping
It occurred to me that psql \dP+ should show the AM of partitioned
tables (and other partitioned rels).
Arguably, this could've been done when \dP was introduced in v12, but
at that point would've shown the AM only for partitioned indexes.
But it makes a lot of sense to do it now that partitioned
On Sat, Apr 20, 2024 at 11:55:46AM +0900, Michael Paquier wrote:
> FYI, as this is an open item, I am planning to wrap that at the
> beginning of next week after a second lookup. If there are any
> comments, feel free.
This one is now fixed with f46bee346c3b, and the open item is marked
as
On Fri, Apr 19, 2024 at 10:41:30AM +0900, Michael Paquier wrote:
> This comes from the contents of the dump for
> regress_pg_dump_table_am_2, that uses heap as table AM. A SET is
> issued for it before dumping regress_pg_dump_table_am_parent and its
> partitions. One trick that I can think of to
On Thu, Apr 18, 2024 at 06:17:56PM +0200, Alvaro Herrera wrote:
> On 2024-Apr-18, Michael Paquier wrote:
>> I was also worrying about a need to dump the protocol version to be
>> able to track the relkind in the toc entries, but a45c78e3284b has
>> already done one. The difference in AM handling
On 2024-Apr-18, Michael Paquier wrote:
> On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:
> > Hmm, maybe we should do a RESET of default_table_access_method before
> > printing the CREATE TABLE to avoid the confusion.
>
> A hard reset would make the business around currTableAM
On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:
> On 2024-Apr-17, Michael Paquier wrote:
>> Yeah, that would be easy enough to track but I was wondering about
>> adding the relkind instead. Still, one thing that I found confusing
>> is the dump generated in this case, as it would
BTW if nothing else, this thread led me to discover a 18-month-old typo
in the Spanish translation of pg_dump:
-msgstr " --no-tablespaces no volcar métodos de acceso de tablas\n"
+msgstr " --no-table-access-method no volcar métodos de acceso de tablas\n"
Oops.
--
Álvaro
On 2024-Apr-17, Michael Paquier wrote:
> Yeah, that would be easy enough to track but I was wondering about
> adding the relkind instead. Still, one thing that I found confusing
> is the dump generated in this case, as it would mix the SET and the
> ALTER TABLE commands so one reading the dumps
On Wed, Apr 17, 2024 at 09:50:02AM +0200, Alvaro Herrera wrote:
> I think it's easy enough to add a "bool ispartitioned" to TableInfo and
> use an ALTER TABLE or rely on the GUC depending on that -- seems easy
> enough.
Yeah, that would be easy enough to track but I was wondering about
adding the
On 2024-Apr-17, Alvaro Herrera wrote:
> Hmm, cannot we simply add a USING clause to the CREATE TABLE command for
> partitioned tables? That would override the
> default_table_access_method, so it should give the correct result, no?
Ah, upthread you noted that pg_restore-time
On 2024-Apr-17, Michael Paquier wrote:
> 2) We could limit these extra ALTER TABLE commands to be generated for
> partitioned tables. This is kind of confusing as resulting dumps
> would mix SET commands for default_table_access_method that would
> affect tables with physical storage, while
On Tue, Apr 16, 2024 at 02:19:56PM +0900, Michael Paquier wrote:
> Actually, I've come up with an idea just after hitting the send
> button: let's use an extra ALTER TABLE SET ACCESS METHOD rather than
> rely on the GUC to set the AM of the partitioned table correctly.
> This extra command should
On Tue, Apr 16, 2024 at 02:14:21PM +0900, Michael Paquier wrote:
> It seems to me that we are going to extend the GUC
> default_table_access_method with a "default" mode to be able to force
> relam to 0 and make a difference with the non-0 case, in the same way
> as ALTER TABLE SET ACCESS METHOD
On Mon, Apr 15, 2024 at 10:46:00AM +0900, Michael Paquier wrote:
> There is no need for a catalog here to trigger the failure, and it
> would have happened as long as a foreign table is used. The problem
> introduced in 374c7a229042 fixed by e2395cdbe83a comes from a thinko
> on my side, my
On Tue, Apr 02, 2024 at 01:06:06AM -0400, Tom Lane wrote:
> AFAICS, e2395cdbe posits that taking exclusive lock on pg_am in the
> middle of a bunch of concurrent regression scripts couldn't possibly
> cause any problems. Really?
There is no need for a catalog here to trigger the failure, and it
Justin Pryzby writes:
> On Sun, Mar 31, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote:
>> I've stumbled upon a test failure caused by the test query added in that
>> commit:
>> +ERROR: deadlock detected
>> +DETAIL: Process 3076180 waits for AccessShareLock on relation 1259 of
>> database
On Sun, Mar 31, 2024 at 12:00:00PM +0300, Alexander Lakhin wrote:
> Hello Alvaro,
>
> 28.03.2024 18:58, Alvaro Herrera wrote:
> > Grumble. I don't like initialization at declare time, so far from the
> > code that depends on the value. But the alternative would have been to
> > assign right
Hello Alvaro,
28.03.2024 18:58, Alvaro Herrera wrote:
Grumble. I don't like initialization at declare time, so far from the
code that depends on the value. But the alternative would have been to
assign right where this blocks starts, an additional line. I pushed it
like you had it.
I've
On 2024-Mar-26, Justin Pryzby wrote:
> Looks right. That's how I originally wrote it, except for the
> "stmt->accessMethod != NULL" case.
>
> I prefered my way - the grammar should refuse to set stmt->accessMethod
> for inappropriate relkinds. And you could assert that.
Hmm, I didn't like
On Thu, Mar 21, 2024 at 01:07:01PM +0100, Alvaro Herrera wrote:
> Given that Michaël is temporarily gone, I propose to push the attached
> tomorrow.
Thanks.
On Tue, Mar 26, 2024 at 12:05:47PM +0100, Alvaro Herrera wrote:
> On 2024-Mar-26, Alexander Lakhin wrote:
>
> > Hello Alvaro,
> >
> >
On 2024-Mar-26, Alexander Lakhin wrote:
> Hello Alvaro,
>
> 21.03.2024 15:07, Alvaro Herrera wrote:
> > Given that Michaël is temporarily gone, I propose to push the attached
> > tomorrow.
>
> Please look at a new anomaly introduced with 374c7a229.
> Starting from that commit, the following
Hello Alvaro,
21.03.2024 15:07, Alvaro Herrera wrote:
Given that Michaël is temporarily gone, I propose to push the attached
tomorrow.
Please look at a new anomaly introduced with 374c7a229.
Starting from that commit, the following erroneous query:
CREATE FOREIGN TABLE fp PARTITION OF pg_am
On Mar 21, 2024, at 13:07, Alvaro Herrera wrote:
> Given that Michaël is temporarily gone, I propose to push the attached
> tomorrow.
Thanks for doing so. I’m wondering whether I should be an author of this patch
at this stage, tbh. I wrote all the tests and rewrote most of the internals to
Given that Michaël is temporarily gone, I propose to push the attached
tomorrow.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
really, I see PHP as like a strange amalgamation of C, Perl, Shell
inflex: you know that "amalgam" means "mixture with mercury",
On 2024-Mar-19, Alvaro Herrera wrote:
> 0001 is Michaël's patch, 0002 are my proposed changes.
Doh, I sent the wrong set of attachments. But I see no reason to post
again: what I attached as 0001 is what I wrote was going to be 0002,
Michaël's patch is already in archives, and the CI tests with
On 2024-Mar-08, Michael Paquier wrote:
> I have spent more time reviewing the whole and the tests (I didn't see
> much value in testing the DEFAULT clause twice for the partitioned
> table case and there is a test in d61a6cad6418), tweaked a few
> comments and the documentation, did an
On Thu, Mar 07, 2024 at 08:02:00PM -0600, Justin Pryzby wrote:
> As you wrote it, you pass the "defaultAccessMethod" bool to
> ATExecSetAccessMethodNoStorage(), which seems odd. Why not just pass
> the target amoid as newAccessMethod ?
+ /*
+* Check that the table access method
On Mon, Mar 04, 2024 at 05:46:56PM +0900, Michael Paquier wrote:
> > Since InvalidOid is already taken, I guess you might need to introduce a
> > boolean flag, like set_relam, indicating that the statement has an
> > ACCESS METHOD clause.
>
> Yes, I don't see an alternative. The default needs a
On Fri, Mar 01, 2024 at 03:03:14PM -0600, Justin Pryzby wrote:
> I think if the user sets something "explicitly", the catalog should
> reflect what they set. Tablespaces have dattablespace, but AMs don't --
> it's a simpler case.
Okay.
> For 001: we don't *need* to support "ALTER SET AM
On Fri, Mar 01, 2024 at 10:56:50AM +0900, Michael Paquier wrote:
> On Thu, Feb 29, 2024 at 08:51:31AM -0600, Justin Pryzby wrote:
> > On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
> >> I have implemented that so as we keep the default, historical
> >> behavior: if pg_class.relam
On Fri, 1 Mar 2024 at 06:15, Michael Paquier wrote:
>
> On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote:
> > I think we should set the AM OID explicitly. Because an important
> > thing to consider is: What behaviour makes sense when later
> > default_table_access_method is
On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote:
> I think we should set the AM OID explicitly. Because an important
> thing to consider is: What behaviour makes sense when later
> default_table_access_method is changed?
> I think if someone sets it explicitly on the partitioned
On Fri, Mar 01, 2024 at 05:43:25AM +0100, Jelte Fennema-Nio wrote:
> I think we should set the AM OID explicitly. Because an important
> thing to consider is: What behaviour makes sense when later
> default_table_access_method is changed?
Per the latest discussion of the thread, we've kind of
On Fri, 1 Mar 2024 at 02:57, Michael Paquier wrote:
> When it comes to partitioned tables, there is a still a tricky case:
> what should we do when a user specifies a non-default value in the SET
> ACCESS METHOD clause and it matches default_table_access_method?
> Should the relam be 0 or should
On Thu, Feb 29, 2024 at 08:51:31AM -0600, Justin Pryzby wrote:
> On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
>> I have implemented that so as we keep the default, historical
>> behavior: if pg_class.relam is 0 for a partitioned table, use the AM
>> defined by
On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
> On Wed, Feb 21, 2024 at 08:46:48AM +0100, Peter Eisentraut wrote:
> > Yes, I think most people agreed that that would be the preferred behavior.
>
> Challenge accepted. As of the patch attached.
Thanks for picking it up. I find
On Wed, Feb 21, 2024 at 08:46:48AM +0100, Peter Eisentraut wrote:
> Yes, I think most people agreed that that would be the preferred behavior.
Challenge accepted. As of the patch attached.
Tablespaces rely MyDatabaseTableSpace to fallback to the database's
default if not specified, but we
On 21.02.24 07:40, Michael Paquier wrote:
This means that all partitioned tables would have pg_class.relam set,
and that relam would never be 0:
- The USING clause takes priority over default_table_access_method.
- If no USING clause, default_table_access_method is the AM used
Any partitions
On Tue, Feb 20, 2024 at 03:47:46PM +0100, Peter Eisentraut wrote:
> It would be helpful if this patch could more extensively document in its
> commit message what semantic changes it makes. Various options of possible
> behaviors were discussed in this thread, but it's not clear which behaviors
>
On 19.07.23 20:13, Justin Pryzby wrote:
On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
What do you think the comment ought to say ? It already says:
src/backend/catalog/heap.c- * Make a dependency
On Thu, Feb 1, 2024 at 10:51 AM Alvaro Herrera wrote:
> I think this works similarly but not identically to tablespace defaults,
> and the difference could be confusing. You seem to have made it so that
> the partitioned table _always_ have a table AM, so the partitions can
> always inherit from
On Thu, Feb 01, 2024 at 04:50:49PM +0100, Alvaro Herrera wrote:
> I think this works similarly but not identically to tablespace defaults,
> and the difference could be confusing. You seem to have made it so that
> the partitioned table _always_ have a table AM, so the partitions can
> always
> @@ -947,20 +947,22 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
> ownerId,
>* a type of relation that needs one, use the default.
>*/
> if (stmt->accessMethod != NULL)
> + accessMethodId = get_table_am_oid(stmt->accessMethod, false);
> + else if
2024-01 Commitfest.
Hi, this patch was marked in CF as "Needs Review" [1], but there has
been no activity on this thread for 6+ months.
Is anything else planned, or can you post something to elicit more
interest in reviews for the latest patch? Otherwise, if nothing
happens then the CF entry
On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
> On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
> > What do you think the comment ought to say ? It already says:
> >
> > src/backend/catalog/heap.c- * Make a dependency link to force
> > the relation
On Sun, Jul 16, 2023 at 09:37:28PM -0500, Justin Pryzby wrote:
> I understand that it's possible for it to be conditional, but I don't
> undertand why it's desirable/important ?
Because it's cheaper on repeated commands, like no CCI necessary.
> It's not conditional in the tablespace code that
On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
> >> Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
> >> update even if ALTER TABLE is defined to use the same table AM as what
> >> is currently set. There is no need to update the relation's pg_class
> >> entry
Have you had a chance to address the comments raised by Michael in his last
review such that a new patch revision can be submitted?
--
Daniel Gustafsson
On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
> What do you think the comment ought to say ? It already says:
>
> src/backend/catalog/heap.c- * Make a dependency link to force
> the relation to be deleted if its
> src/backend/catalog/heap.c- * access
On Thu, May 25, 2023 at 03:49:12PM +0900, Michael Paquier wrote:
> looking at the patch. Here are a few comments.
...
> * No need to add an explicit dependency for the toast table, as the
> * main table depends on it.
> */
> - if (RELKIND_HAS_TABLE_AM(relkind) &&
On Mon, Apr 24, 2023 at 07:18:54PM -0500, Justin Pryzby wrote:
> On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:
>> Actually .. I think it'd be a mistake if the relam needed to be
>> interpretted differently for partitioned tables than other relkinds.
>>
>> I updated the patch to
On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:
> What more points ? There's nothing that's been raised here. In fact,
> your message last week is the first comment since last June ..
When I wrote this message, I felt like this may still be missing
something in the area of
On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:
> On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote:
> > On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:
> > > On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> > > > Did you check dump and
On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote:
> On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:
> > On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> > > Did you check dump and restore flows with partition
> > > trees and --no-table-access-method?
On Tue, Mar 28, 2023 at 02:56:28PM +0900, Michael Paquier wrote:
> Hmm. This is a good point. It is true that the patch feels
> incomplete on this side. I don't see why we could not be flexible,
> and allow a value of 0 in a partitioned table's relam to mean that we
> would pick up the database
On Mon, Mar 27, 2023 at 11:34:35PM -0500, Justin Pryzby wrote:
> I realized that one difference with tablespaces is that, as written,
> partitioned tables will *always* have an AM specified, and partitions
> will never use default_table_access_method. Is that what's intended ?
>
> Or do we need
On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:
> On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> > Did you check dump and restore flows with partition
> > trees and --no-table-access-method? Perhaps there should be
> > some regression tests with partitioned
On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> Did you check dump and restore flows with partition
> trees and --no-table-access-method? Perhaps there should be
> some regression tests with partitioned tables?
I was looking at the patch, and as I suspected the dumps generated
On Thu, Jun 09, 2022 at 11:21:58AM -0700, Soumyadeep Chakraborty wrote:
> Thanks. Fixed and rebased.
I think that I am OK with the concept of this patch to use a
partitioned table's relam as a reference when creating a partition
rather than relying on the default GUC, in a way similar to
On Wed, May 18, 2022 at 6:26 PM Zhihong Yu wrote:
> + accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
>
> - /* look up the access method, verify it is for a table */
> - if (accessMethod != NULL)
> - accessMethodId = get_table_am_oid(accessMethod, false);
> + if
On Wed, May 18, 2022 at 5:49 PM Soumyadeep Chakraborty <
soumyadeep2...@gmail.com> wrote:
>
> On Wed, May 18, 2022 at 4:14 PM Justin Pryzby
> wrote:
>
> > I didn't look closely yet, but this comment is wrong:
> >
> > + * Since these have no storage the tablespace can be updated with a
> simple
>
On Wed, May 18, 2022 at 4:14 PM Justin Pryzby wrote:
> I didn't look closely yet, but this comment is wrong:
>
> + * Since these have no storage the tablespace can be updated with a
simple
> + * metadata only operation to update the tablespace.
Good catch. Fixed.
> It'd be convenient if
Thanks for copying me.
I didn't look closely yet, but this comment is wrong:
+ * Since these have no storage the tablespace can be updated with a simple
Hi,
+ tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;
- /* look up the access method, verify it is for a table */
- if (accessMethod != NULL)
- accessMethodId = get_table_am_oid(accessMethod, false);
+
Hello,
This is a fresh thread to continue the discussion on ALTER TABLE SET
ACCESS METHOD when applied to partition roots, as requested.
Current behavior (HEAD):
CREATE TABLE am_partitioned(x INT, y INT)
PARTITION BY hash (x);
ALTER TABLE am_partitioned SET ACCESS METHOD heap2;
ERROR:
67 matches
Mail list logo
