RE: Disable WAL logging to speed up data loading - Mailing list pgsql-hackers

Hi

This time, I updated my patch to address comments below only.
Please have a look at updated one.

On Thursday, Nov 19, 2020 4:51 PM Tsunakawa, Takayuki <tsunakawa.takay@fujitsu.com>
> (1)
>  #define RelationNeedsWAL(relation)
>                     \
> +    (wal_level != WAL_LEVEL_NONE &&
>                         \
>      ((relation)->rd_rel->relpersistence ==
> RELPERSISTENCE_PERMANENT &&    \
>       (XLogIsNeeded() ||
>                             \
>        (relation->rd_createSubid == InvalidSubTransactionId &&
>         \
> -       relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))
> +       relation->rd_firstRelfilenodeSubid ==
> InvalidSubTransactionId))))
> 
> You can remove one pair of parentheses for readability as follows:
> 
>  #define RelationNeedsWAL(relation)
>                     \
> +    (wal_level != WAL_LEVEL_NONE &&
>                         \
>      (relation)->rd_rel->relpersistence ==
> RELPERSISTENCE_PERMANENT &&    \
>       (XLogIsNeeded() ||
>                             \
>        (relation->rd_createSubid == InvalidSubTransactionId &&
>         \
> -       relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))
> +       relation->rd_firstRelfilenodeSubid == InvalidSubTransactionId)))
Done. 

> (2)
>       /*
> +     * Detect if the server previously crashed under wal_level='none' or
> not.
> +     */
> +    if (ControlFile->wal_level == WAL_LEVEL_NONE &&
> +        (ControlFile->state != DB_SHUTDOWNED &&
> ControlFile->state != DB_SHUTDOWNED_IN_RECOVERY))
> +        ereport(ERROR,
> +                (errmsg("detected an unexpected server
> shutdown when WAL logging was disabled"),
> +                 errhint("It looks like you need to deploy a
> new cluster from your full backup again.")));
> +
> +    /*
>       * Check that contents look valid.
>       */
>      if (!XRecOffIsValid(ControlFile->checkPoint))
> 
> I think the above safeguard should be placed right before the following code,
> because it's better to first check control file contents and emit the message
> for the previous shutdown state.
> 
> #ifdef XLOG_REPLAY_DELAY
>     if (ControlFile->state != DB_SHUTDOWNED)
>         pg_usleep(60000000L);
> #endif
Thank you. Perfectly makes sense. Fixed.


> (3)
> @@ -449,6 +449,13 @@ XLogInsert(RmgrId rmid, uint8 info)
>          return EndPos;
>      }
> 
> +    /* Issues WAL only for XLOG resources */
> +    if (wal_level == WAL_LEVEL_NONE && rmid != RM_XLOG_ID)
> +    {
> +        XLogResetInsertion();
> +        return GetLatestCheckPointLSN();
> +    }
> 
> It should be correct to use GetXLogInsertRecPtr() instead of
> GetLatestCheckPointLSN(), because what XLogInsert() should return is the
> end position of last WAL record inserted (= start position of next WAL).
Ah, sorry. My patch was not correct.


> Why don't we add transaction completion WAL records?  That is, add
> "rmid != RM_XACT_ID" to the if condition.  What we really want to eliminate
> for performance is the data modification WAL records.  This might allow us
> to insert special checks to prevent PREPARE TRANSACTION and other stuff.
I apologize that you mentioned this point in your past review but I took it wrongly.
I fixed the condition and the comment for it.


> What happens without these checks? 
> (4)
> @@ -404,6 +404,10 @@
> pg_logical_emit_message_bytea(PG_FUNCTION_ARGS)
>      bytea       *data = PG_GETARG_BYTEA_PP(2);
>      XLogRecPtr    lsn;
> 
> +    if (wal_level < WAL_LEVEL_REPLICA)
> +        ereport(ERROR,
> +                errmsg("propagating a message requires
> wal_level \"replica\" or \"logical\""));
> +
>      lsn = LogLogicalMessage(prefix, VARDATA_ANY(data),
> VARSIZE_ANY_EXHDR(data),
>                              transactional);
>      PG_RETURN_LSN(lsn);
> 
> @@ -192,6 +192,10 @@ replorigin_check_prerequisites(bool check_slots,
> bool recoveryOK)
> 
>     (errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
>                   errmsg("cannot manipulate replication
> origins during recovery")));
> 
> +    if (wal_level < WAL_LEVEL_REPLICA)
> +        ereport(ERROR,
> +                errmsg("creating replication origins requires
> wal_level \"replica\" or \"logical\""));
> +
>  }
Those functions doesn't do something meaningful when wal_level <= minimal.
For example, pg_logical_emit_message_bytea calls XLogInsert with RM_LOGICALMSG_ID rmid.
This doesn't generate WAL during wal_level=none,
which makes the usage of the function nonsense.

But, the latest patch I attached removed this part of modification,
because this can be applied to wal_level = minimal also and
I should make the patch focus on its purpose only.


> @@ -625,6 +625,9 @@ standard_ProcessUtility(PlannedStmt *pstmt,
>                          break;
> 
>                      case TRANS_STMT_PREPARE:
> +                        if (wal_level ==
> WAL_LEVEL_NONE)
> +                            ereport(ERROR,
> +
>     errmsg("cannot execute PREPARE TRANSACTION when WAL
> logging is disabled"));
Usually, this safeguard is not needed for PREPARE TRANSACTION.
But, when an unexpected crash happens, user needs
to recreate the cluster from the backup and
execute the operations that are executed into the crashed server again
if the user sets wal_level=none.

While 2PC guarantees that after issuing PREPARE TRANSACTION,
the processing or the contents of the transaction becomes *secure*,
setting wal_level=none makes the server never start up again if a database crash occurs.
In other words, this safeguard of the new wal_level damages
the important aspect of PREPARE TRANSACTION's functionality, in my opinion.

I don't think this is what it should be.

Therefore, I don't want users to utilize the PREPARE TRANSACTION still
in order to prevent that user thinks that they can use PREPARE TRANSACTION
safely during wal_level=none and execute it.
Did it make sense ?

Lastly, I fixed the documentation related to the types of generated WAL as well.

Best,
    Takamichi Osumi