Re: In-placre persistance change of a relation - Mailing list pgsql-hackers

Thanks! Version 20 is attached.


At Wed, 30 Mar 2022 08:44:02 -0500, Justin Pryzby <pryzby@telsasoft.com> wrote in 
> On Tue, Mar 01, 2022 at 02:14:13PM +0900, Kyotaro Horiguchi wrote:
> > Rebased on a recent xlog refactoring.
> 
> It'll come as no surprise that this neds to be rebased again.
> At least a few typos I reported in January aren't fixed.
> Set to "waiting".

Oh, I'm sorry for overlooking it. It somehow didn't show up on my
mailer.

> I started looking at this and reviewed docs and comments again.
> 
> > +typedef struct PendingCleanup
> > +{
> > +    RelFileNode relnode;        /* relation that may need to be deleted */
> > +    int            op;                /* operation mask */
> > +    bool        bufpersistence;    /* buffer persistence to set */
> > +    int            unlink_forknum;    /* forknum to unlink */
> 
> This can be of data type "ForkNumber"

Right. Fixed.

> > +     * We are going to create an init fork. If server crashes before the
> > +     * current transaction ends the init fork left alone corrupts data while
> > +     * recovery.  The mark file works as the sentinel to identify that
> > +     * situation.
> 
> s/while/during/

This was in v17, but dissapeared in v18.

> > +     * index-init fork needs further initialization. ambuildempty shoud do
> 
> should (I reported this before)
> 
> > +    if (inxact_created)
> > +    {
> > +        SMgrRelation srel = smgropen(rnode, InvalidBackendId);
> > +
> > +        /*
> > +         * INIT forks never be loaded to shared buffer so no point in dropping
> 
> "are never loaded"

If was fixed in v18.

> > +    elog(DEBUG1, "perform in-place persistnce change");
> 
> persistence (I reported this before)

Sorry. Fixed.

> > +        /*
> > +         * While wal_level >= replica, switching to LOGGED requires the
> > +         * relation content to be WAL-logged to recover the table.
> > +         * We don't emit this fhile wal_level = minimal.
> 
> while (or "if")

There are "While" and "fhile". I changed the latter to "if".

> > +             * The relation is persistent and stays remain persistent. Don't
> > +             * drop the buffers for this relation.
> 
> "stays remain" is redundant (I reported this before)

Thanks. I changed it to "stays persistent".

> > +            if (unlink(rm_path) < 0)
> > +                ereport(ERROR,
> > +                        (errcode_for_file_access(),
> > +                         errmsg("could not remove file \"%s\": %m",
> > +                                rm_path)));
> 
> The parens around errcode are unnecessary since last year.
> I suggest to avoid using them here and elsewhere.

It is just moved from elsewhere without editing, but of course I can
do that.  I didn't know about that change of ereport and not found the
corresponding commit, but I found that Tom mentioned that change.

https://www.postgresql.org/message-id/flat/5063.1584641224%40sss.pgh.pa.us#63e611c30800133bbddb48de857668e8
> Now that we can rely on having varargs macros, I think we could
> stop requiring the extra level of parentheses, ie instead of
...
>         ereport(ERROR,
>                 errcode(ERRCODE_DIVISION_BY_ZERO),
>                 errmsg("division by zero"));
> 
> (The old syntax had better still work, of course.  I'm not advocating
> running around and changing existing calls.)

I changed all ereport calls added by this patch to this style.

> > +             * And we may have SMGR_MARK_UNCOMMITTED file.  Remove it if the
> > +             * fork files has been successfully removed. It's ok if the file
> 
> file

Fixed.

> > +     <para>
> > +      All tables in the current database in a tablespace can be changed by using
> 
> given tablespace

I did /database in a tablespace/database in the given tablespace/. Is
it right?

> > +      the <literal>ALL IN TABLESPACE</literal> form, which will lock all tables
> 
> which will first lock
>
> > +      to be changed first and then change each one.  This form also supports
> 
> remove "first" here

This is almost a dead copy of the description of SET TABLESPACE. This
change makes the two almost the same description vary slightly in that
wordings.  Anyway I did that as suggested only for the part this patch
adds in this version.

> > +      <literal>OWNED BY</literal>, which will only change tables owned by the
> > +      roles specified.  If the <literal>NOWAIT</literal> option is specified
> 
> specified roles.
> is specified, (comma)

This is the same as above. I did that but it makes the description
differ from another almost-the-same description.

> > +      then the command will fail if it is unable to acquire all of the locks
> 
> if it is unable to immediately acquire
>
> > +      required immediately.  The <literal>information_schema</literal>
> 
> remove immediately

Ditto.

> > +      relations are not considered part of the system catalogs and will be
> 
> I think you need to first say that "relations in the pg_catalog schema cannot
> be changed".

Mmm. I don't agree on this.  Aren't such "exceptions"-ish descriptions
usually placed after the descriptions of how the feature works? This
is also the same structure with SET TABLESPACE.

> in patch 2/2:
> typo: persistene

Hmm. Bad. I checked the spellings of the whole patches and found some
typos.

+                     * The crashed trasaction did SET UNLOGGED. This relation
+                     * is restored to a LOGGED relation.
s/trasaction/transaction/

+                errmsg("could not crete mark file \"%s\": %m", path));
s/crete/create/

Then rebased on 9c08aea6a3 then pgindent'ed.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center