ChinmaySKulkarni commented on a change in pull request #935:
URL: https://github.com/apache/phoenix/pull/935#discussion_r520170305
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java
##########
@@ -399,6 +401,19 @@ public MetaDataMutationResult
validateAndAddMetadata(PTable table, byte[][] rowK
rowKeyMetaData[TABLE_NAME_INDEX])));
}
}
+ if (isAddingColumns) {
+ //We're changing the application-facing schema by adding a column,
so update the DDL
+ // timestamp
+ long serverTimestamp = EnvironmentEdgeManager.currentTimeMillis();
+
additionalTableMetadataMutations.add(MetaDataUtil.getLastDDLTimestampUpdate(tableHeaderRowKey,
+ clientTimeStamp, serverTimestamp));
+ for (PTable viewTable : childViews) {
Review comment:
In splittable-SYSTEM.CATALOG world, we avoid making synchronous changes
to child views when the parent changes, so if a column is added to the parent,
we wouldn't add that to each child view's metadata. Instead on resolving the
view, we'd combine the parent columns and inherit them that way. This was done
for scalability in case child views span many SYSCAT regions. I'm wondering if
we can use similar inheritance logic for lastDDLTs of child views.
This becomes more complicated for diverged views. In that case, any columns
added to a parent view/physical table are inherited by its child views only if
they haven't diverged. In those cases, the LastDDLTs for diverged views ideally
shouldn't even be modified if a column is added to its parent.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java
##########
@@ -94,7 +94,7 @@
public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_13_0 =
MIN_SYSTEM_TABLE_TIMESTAMP_4_11_0;
public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0 =
MIN_TABLE_TIMESTAMP + 28;
public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_15_0 =
MIN_TABLE_TIMESTAMP + 29;
- public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 =
MIN_TABLE_TIMESTAMP + 31;
+ public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 =
MIN_TABLE_TIMESTAMP + 33;
Review comment:
Why are we changing this?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DropColumnMutator.java
##########
@@ -268,7 +272,20 @@ public MetaDataMutationResult
validateAndAddMetadata(PTable table,
}
}
- tableMetaData.addAll(additionalTableMetaData);
+ if (isDroppingColumns) {
+ //We're changing the application-facing schema by dropping a
column, so update the DDL
+ // timestamp to current _server_ timestamp
+ long serverTimestamp = EnvironmentEdgeManager.currentTimeMillis();
+
additionalTableMetaData.add(MetaDataUtil.getLastDDLTimestampUpdate(tableHeaderRowKey,
+ clientTimeStamp, serverTimestamp));
+ for (PTable viewTable : childViews) {
Review comment:
Same concerns here with parent column inheritance and diverged views,
etc.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java
##########
@@ -399,6 +401,19 @@ public MetaDataMutationResult
validateAndAddMetadata(PTable table, byte[][] rowK
rowKeyMetaData[TABLE_NAME_INDEX])));
}
}
+ if (isAddingColumns) {
+ //We're changing the application-facing schema by adding a column,
so update the DDL
+ // timestamp
+ long serverTimestamp = EnvironmentEdgeManager.currentTimeMillis();
+
additionalTableMetadataMutations.add(MetaDataUtil.getLastDDLTimestampUpdate(tableHeaderRowKey,
+ clientTimeStamp, serverTimestamp));
+ for (PTable viewTable : childViews) {
Review comment:
Another complication is, pre-4.15 clients do not have this logic to just
store view-specific columns. They don't have EXCLUDED_COLUMN linking rows
either. The parent column combining logic in those cases is different too.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
##########
@@ -2586,4 +2588,18 @@ public static boolean isNoUpgradeSet(Properties props) {
public static void doNotUpgradeOnFirstConnection(Properties props) {
props.setProperty(DO_NOT_UPGRADE, String.valueOf(true));
}
+
+ //When upgrading to Phoenix 4.16, make each existing table's DDL timestamp
equal to its last
+ // updated row timestamp.
+ public static void bootstrapLastDDLTimestamp(PhoenixConnection
metaConnection) throws SQLException {
+ String pkCols = TENANT_ID + ", " + TABLE_SCHEM +
+ ", " + TABLE_NAME + ", " + COLUMN_NAME + ", " + COLUMN_FAMILY;
+ String upsertSql =
+ "UPSERT INTO " + SYSTEM_CATALOG_NAME + " (" + pkCols + ", " +
+ LAST_DDL_TIMESTAMP + ")" + " " +
+ "SELECT " + pkCols + ", PHOENIX_ROW_TIMESTAMP() FROM " +
SYSTEM_CATALOG_NAME + " " +
+ "WHERE " + TABLE_TYPE + " " + " in " + "('u','v')";
Review comment:
nit: Instead of using `u` and `v`, use PTableType.. API
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -3707,15 +3709,20 @@ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection met
metaConnection = addColumnsIfNotExists(
metaConnection,
PhoenixDatabaseMetaData.SYSTEM_CATALOG,
- MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 - 1,
+ MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 - 2,
Review comment:
I'm a little unclear on these changes. Shouldn't all these 4.16-specific
columns be added at the 4.16 ts rather than `4.16 ts - something`?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
##########
@@ -2586,4 +2588,18 @@ public static boolean isNoUpgradeSet(Properties props) {
public static void doNotUpgradeOnFirstConnection(Properties props) {
props.setProperty(DO_NOT_UPGRADE, String.valueOf(true));
}
+
+ //When upgrading to Phoenix 4.16, make each existing table's DDL timestamp
equal to its last
+ // updated row timestamp.
+ public static void bootstrapLastDDLTimestamp(PhoenixConnection
metaConnection) throws SQLException {
+ String pkCols = TENANT_ID + ", " + TABLE_SCHEM +
Review comment:
Can we add a log before and after this `UPSERT SELECT` to help track the
upgrade progress?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
##########
@@ -2037,7 +2049,10 @@ public void createTable(RpcController controller,
CreateTableRequest request,
// view's property in case they are different from the
parent
ViewUtil.addTagsToPutsForViewAlteredProperties(tableMetadata, parentTable);
}
-
+ //set the last DDL timestamp to the current server time since
we're creating the
+ // table
+
tableMetadata.add(MetaDataUtil.getLastDDLTimestampUpdate(tableKey,
+ clientTimeStamp,
EnvironmentEdgeManager.currentTimeMillis()));
Review comment:
Do we want to restrict this to just tables and views i.e. 'u' and 'v'
table_types? The upgrade code only adds a ts for existing tables and views, but
not indexes and SYSTEM tables, but here we do it for all types. There will be
inconsistency in that case between an index created before upgrading (no ts) vs
an index created after upgrading (has ts), not to mention fresh clusters will
have a ts for all entities.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
##########
@@ -2586,4 +2588,18 @@ public static boolean isNoUpgradeSet(Properties props) {
public static void doNotUpgradeOnFirstConnection(Properties props) {
props.setProperty(DO_NOT_UPGRADE, String.valueOf(true));
}
+
+ //When upgrading to Phoenix 4.16, make each existing table's DDL timestamp
equal to its last
+ // updated row timestamp.
+ public static void bootstrapLastDDLTimestamp(PhoenixConnection
metaConnection) throws SQLException {
+ String pkCols = TENANT_ID + ", " + TABLE_SCHEM +
+ ", " + TABLE_NAME + ", " + COLUMN_NAME + ", " + COLUMN_FAMILY;
+ String upsertSql =
+ "UPSERT INTO " + SYSTEM_CATALOG_NAME + " (" + pkCols + ", " +
+ LAST_DDL_TIMESTAMP + ")" + " " +
+ "SELECT " + pkCols + ", PHOENIX_ROW_TIMESTAMP() FROM " +
SYSTEM_CATALOG_NAME + " " +
+ "WHERE " + TABLE_TYPE + " " + " in " + "('u','v')";
Review comment:
Also, can we add a comment about why we don't do this for system tables
and indexes? Might be better to invert the condition and say `WHERE TABLE_TYPE
NOT IN ('s', i')`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
