Re: In-placre persistance change of a relation

Thanks! Version 20 is attached.

At Wed, 30 Mar 2022 08:44:02 -0500, Justin Pryzby <pryzby(at)telsasoft(dot)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